Add HTTP client diagnostics_channel event support#24375
Add HTTP client diagnostics_channel event support#24375
Conversation
|
Updated 3:05 PM PT - Nov 6th, 2025
❌ @alii, your commit a178c24 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 24375That installs a local version of the PR into your bun-24375 --bun |
Implements diagnostics_channel events for HTTP client requests to match Node.js behavior. This enables observability tools like Sentry to monitor HTTP requests via diagnostics channels. Events added: - http.client.request.created: Emitted when ClientRequest is created - http.client.request.start: Emitted when fetch() starts - http.client.request.error: Emitted on request errors - http.client.response.finish: Emitted when response completes Fixes support for Sentry's HTTP instrumentation which relies on these events to generate spans. Reference: getsentry/sentry-javascript#17779 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
robobun
force-pushed
the
claude/add-http-diagnostics-channel-events
branch
from
6d4829f to
f688d1e
Compare
This test verifies that all four HTTP client diagnostics_channel events are properly emitted: - http.client.request.created - http.client.request.start - http.client.request.error - http.client.response.finish Based on Node.js's test-diagnostics-channel-http.js but focused only on client events (server events not yet implemented in Bun). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughAdds Diagnostics Channel integration to the HTTP client, publishing events for request creation, request start, request errors, and response finish. Emissions are guarded by subscriber checks and do not change exported/public API signatures. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (8)test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/node/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/node/test/**📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.{js,ts}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/js/node/test/{parallel,sequential}/*.js📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.{ts,tsx,js,jsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/js/{bun,node}/**📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (13)📓 Common learnings📚 Learning: 2025-10-18T05:23:24.403ZApplied to files:
📚 Learning: 2025-10-19T04:55:33.099ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-09-08T00:41:12.052ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-10-12T02:22:34.373ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
📚 Learning: 2025-09-20T00:58:38.042ZApplied to files:
📚 Learning: 2025-10-19T04:55:27.213ZApplied to files:
📚 Learning: 2025-09-19T21:17:04.690ZApplied to files:
📚 Learning: 2025-08-30T00:12:56.803ZApplied to files:
📚 Learning: 2025-08-30T00:09:39.100ZApplied to files:
🔇 Additional comments (3)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/js/node/_http_client.ts(7 hunks)test/js/node/diagnostics_channel/http-client-diagnostics.test.ts(1 hunks)test/js/node/test/parallel/test-diagnostics-channel-http-client.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (13)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
src/js/node/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
Place Node.js compatibility modules (e.g., node:fs, node:path) under node/
Files:
src/js/node/_http_client.ts
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}
📄 CodeRabbit inference engine (src/js/CLAUDE.md)
src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js}: Use require() with string literals only (no dynamic requires)
Do not use ESM import syntax; write modules as CommonJS with export default { ... }
Export via export default {} for modules
Use .$call and .$apply; never use .call or .apply
Prefer JSC intrinsics/private $ APIs for performance (e.g., $Array.from, map.$set, $newArrayWithSize, $debug, $assert)
Validate callbacks with $isCallable and throw $ERR_INVALID_ARG_TYPE with the correct parameter name and expected type
Use process.platform and process.arch for platform detection (rely on inlining/dead-code elimination)
Files:
src/js/node/_http_client.ts
test/js/node/test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
test/js/node/test/{parallel,sequential}/*.js
📄 CodeRabbit inference engine (test/CLAUDE.md)
For
test/js/node/test/{parallel,sequential}/*.js(no .test extension), run withbun bd <file>instead ofbun bd test <file>
Files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
🧠 Learnings (18)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tssrc/js/node/_http_client.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/bun/**/*.{js,ts} : Place Bun API tests under test/js/bun/, separated by category (e.g., test/js/bun/glob/)
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js,test/**/*-fixture.ts} : Use `using`/`await using` for resource cleanup with Bun APIs (e.g., `Bun.spawn`, `Bun.listen`, `Bun.serve`)
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-25T17:20:19.041Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: test/js/bun/telemetry/server-header-injection.test.ts:5-20
Timestamp: 2025-10-25T17:20:19.041Z
Learning: In the Bun telemetry codebase, tests are organized into two distinct layers: (1) Internal API tests in test/js/bun/telemetry/ use numeric InstrumentKind enum values to test Zig↔JS injection points and low-level integration; (2) Public API tests in packages/bun-otel/test/ use string InstrumentKind values ("http", "fetch", etc.) to test the public-facing BunSDK and instrumentation APIs. This separation allows internal tests to use efficient numeric enums for refactoring flexibility while the public API maintains a developer-friendly string-based interface.
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/{bun,node}/** : Organize unit tests by module under `/test/js/bun/` and `/test/js/node/`
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : When spawning Bun in tests, use `bunExe()` and `bunEnv` from `harness`
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-10-19T04:55:27.213Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-88
Timestamp: 2025-10-19T04:55:27.213Z
Learning: In the Bun.telemetry._node_binding API for Node.js http.createServer compatibility, there are exactly two hooks: handleIncomingRequest(req, res) called once per request (returns a request ID), and handleWriteHead(res, statusCode) called once when headers are sent (explicit or implicit). The old handleRequestFinish hook no longer exists after architectural refactoring. Both hooks are tested in test/js/bun/http/node-telemetry.test.ts.
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.tssrc/js/node/_http_client.tstest/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts
📚 Learning: 2025-09-05T20:20:04.858Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:163-178
Timestamp: 2025-09-05T20:20:04.858Z
Learning: Node-redis PUB/SUB callback error handling is broken - when a callback throws an error, it first emits the error as "Redis Client Error", then enters an undefined state with cascading internal errors like "undefined is not an object (evaluating 'this.#waitingForReply.head.value')", making the client unusable for further processing.
Applied to files:
src/js/node/_http_client.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/client-fixture.mjs : Client subprocess logic (page load, IPC, error/reload coordination) should live in client-fixture.mjs and be driven by Client
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http-client.js
🧬 Code graph analysis (1)
src/js/node/_http_client.ts (1)
test/js/node/test/parallel/test-diagnostics-channel-http-client.js (1)
dc(6-6)
🔇 Additional comments (12)
src/js/node/_http_client.ts (6)
8-14: LGTM! Diagnostics channel setup follows Node.js conventions.The four HTTP client diagnostics channels match Node.js's diagnostics_channel API, enabling observability tools like Sentry to monitor HTTP requests.
70-81: LGTM! Error event emission follows best practices.Publishing to diagnostics_channel before emitting the user-facing error event ensures observability tools can intercept errors. The
hasSubscriberscheck correctly avoids overhead when no channels are subscribed.
392-397: LGTM! Start event correctly emitted before fetch begins.The event is published at the right point in the lifecycle—immediately before the actual fetch() call—with the correct payload format.
432-441: LGTM! Response finish event correctly emitted with helpful clarification.The comment on lines 433-435 is valuable—it clarifies that Node.js emits "response.finish" when response headers arrive, not when the body completes. This matches Node.js behavior and prevents future confusion.
972-978: LGTM! Created event correctly emitted at end of constructor.Placement ensures all ClientRequest initialization is complete before the diagnostics_channel event is published, and it will always precede the "start" event in the lifecycle.
498-503: No duplicate error emissions detected. The three publication points handle distinct, mutually-exclusive error scenarios.Analysis of error paths:
- send() try-catch (lines 602-607): Catches synchronous errors during
startFetch(body)call; if this fires, the fetch promise is never created- fetch().catch() (lines 498-503): Handles promise rejections only when
startFetchsucceeds; this cannot run if the try-catch at line 595 fires- emitErrorEventNT (lines 72-77): Called from response header validation in separate control flow, unrelated to request-sending errors
Each error takes exactly one path, making duplicate publications impossible.
test/js/node/test/parallel/test-diagnostics-channel-http-client.js (3)
1-10: LGTM! Test setup follows Node.js conventions.This is a vendored Node.js test, so the use of
common,addresses.INVALID_HOST, and other Node.js test utilities is expected and appropriate. The type guard functions provide clear validation in the channel subscribers.
12-31: LGTM! Channel subscriptions correctly validate event counts and payloads.The use of
common.mustCallwith specific counts ensures that events are emitted the expected number of times. The payload validations confirm the correct structure for each event type.
33-55: LGTM! Test execution covers both error and success paths.The test correctly exercises both the error scenario (invalid host) and the success scenario (valid request), ensuring all diagnostics_channel events are tested. Server cleanup is properly handled.
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts (3)
5-87: LGTM! Comprehensive test for basic GET request flow.The test correctly:
- Uses
port: 0for the server (per coding guidelines)- Validates event order (created → start → finish)
- Checks payload structure for each event
- Properly cleans up the server after completion
143-185: LGTM! http.get test confirms diagnostics events work across HTTP methods.The test correctly verifies that
http.get(a convenience method) emits the same diagnostics_channel events ashttp.request, ensuring consistent observability.
187-255: LGTM! POST with body test ensures diagnostics events work with request bodies.The test confirms that diagnostics_channel events are emitted correctly for requests with bodies, covering an important use case for observability tools.
| // Make a request to a non-existent server | ||
| await new Promise<void>((resolve, reject) => { | ||
| const req = http.request( | ||
| { | ||
| hostname: "localhost", | ||
| port: 1, // Port 1 should refuse connection | ||
| path: "/", | ||
| method: "GET", | ||
| }, | ||
| () => { | ||
| reject(new Error("Should not receive response")); | ||
| }, | ||
| ); | ||
|
|
||
| req.on("error", err => { | ||
| // Error is expected | ||
| expect(err).toBeDefined(); | ||
| resolve(); | ||
| }); | ||
|
|
||
| req.end(); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a more reliable error trigger than port 1.
While port 1 typically refuses connections, it's not guaranteed to be closed on all systems or network configurations. Consider using a closed server or a non-routable IP address (e.g., 240.0.0.0) for more reliable error testing across environments.
Alternative approach using a closed server:
- // Make a request to a non-existent server
+ // Create a server and close it immediately to guarantee connection refused
+ const closedServer = http.createServer();
+ await new Promise<void>(resolve => {
+ closedServer.listen(0, () => {
+ const { port } = closedServer.address() as any;
+ closedServer.close(() => resolve());
+
- await new Promise<void>((resolve, reject) => {
- const req = http.request(
- {
- hostname: "localhost",
- port: 1, // Port 1 should refuse connection
- path: "/",
- method: "GET",
- },
+ // Now make request to the closed port
+ const req = http.request(
+ {
+ hostname: "localhost",
+ port: port,
+ path: "/",
+ method: "GET",
+ },
+ () => {
+ reject(new Error("Should not receive response"));
+ },
+ );
+
+ req.on("error", err => {
+ // Error is expected
+ expect(err).toBeDefined();
+ resolve();
+ });
+
+ req.end();
+ });
+ });
- () => {
- reject(new Error("Should not receive response"));
- },
- );
-
- req.on("error", err => {
- // Error is expected
- expect(err).toBeDefined();
- resolve();
- });
-
- req.end();
- });Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
test/js/node/test/parallel/test-diagnostics-channel-http.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/js/node/test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Vendored Node.js tests under test/js/node/test/ are exceptions and do not follow Bun’s test style
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/js/node/test/{parallel,sequential}/*.js
📄 CodeRabbit inference engine (test/CLAUDE.md)
For
test/js/node/test/{parallel,sequential}/*.js(no .test extension), run withbun bd <file>instead ofbun bd test <file>
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
🧠 Learnings (13)
📓 Common learnings
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-09-08T00:41:12.052Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-09-08T00:41:12.052Z
Learning: Follow existing patterns in similar V8 classes, add comprehensive Node.js parity tests, update all symbol files, and document any special behavior
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-10-19T04:55:27.213Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-88
Timestamp: 2025-10-19T04:55:27.213Z
Learning: In the Bun.telemetry._node_binding API for Node.js http.createServer compatibility, there are exactly two hooks: handleIncomingRequest(req, res) called once per request (returns a request ID), and handleWriteHead(res, statusCode) called once when headers are sent (explicit or implicit). The old handleRequestFinish hook no longer exists after architectural refactoring. Both hooks are tested in test/js/bun/http/node-telemetry.test.ts.
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/client-fixture.mjs : Client subprocess logic (page load, IPC, error/reload coordination) should live in client-fixture.mjs and be driven by Client
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Dev server tests should be thorough yet concise to validate hot-reloading robustness, correctness, and reliability
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Always check exit codes and error scenarios in tests (e.g., spawned processes should assert non-zero on failure)
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Explicitly assert client console logs with c.expectMessage; unasserted logs should not appear
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-10-26T05:04:50.692Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.692Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests, always use port: 0; do not hardcode ports or invent random port functions
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/{test/**/*.test.{ts,js,jsx,tsx,mjs,cjs},test/js/node/test/{parallel,sequential}/*.js} : Never hardcode port numbers; use `port: 0` to get a random port
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/bundle.test.ts : bundle.test.ts should contain DevServer-specific bundling tests
Applied to files:
test/js/node/test/parallel/test-diagnostics-channel-http.js
🔇 Additional comments (4)
test/js/node/test/parallel/test-diagnostics-channel-http.js (4)
1-13: LGTM: Imports and type checking setup.The imports and type checking helper functions follow standard Node.js test patterns and are appropriate for validating diagnostics_channel event payloads.
15-30: LGTM: Client event subscriptions.The client event subscriptions correctly validate the three key HTTP client diagnostics events (
http.client.request.start,http.client.request.error,http.client.response.finish) with appropriate call counts and type assertions. The expected call counts align with the test flow (2 requests, 1 error, 1 successful response).
32-62: LGTM: Server event subscriptions.The server event subscriptions properly validate server-side diagnostics events with correct call counts and comprehensive payload assertions. The single call count for each server event correctly reflects that only the valid request reaches the server (the invalid request errors before connection).
69-87: LGTM: Test execution flow.The test execution properly exercises both error and success paths:
- Creates server with dynamic port allocation (
port: 0)- Triggers an invalid request to test error diagnostics
- Waits for the error before proceeding
- Makes a valid request to test successful flow
- Properly cleans up by closing the server
The structure ensures comprehensive coverage of HTTP client diagnostics events in both error and success scenarios.
|
Hey @Jarred-Sumner @alii (apologies for the direct ping) is there anything we can help with to get this merged? A bunch of Sentry and OpenTelemetry instrumentation relies on these diagnostic channel events. |
|
one more humble request to get this across the finish line - just moved over to bun runtime (happy!) but lost all sentry scoped tracing (sad!) |
|
@Jarred-Sumner, can we please have someone look at this? This is breaking Sentry for many Bun users. |
5 participants
Summary
Implements
diagnostics_channelevents for HTTP client requests to match Node.js behavior. This enables observability tools like Sentry to monitor HTTP requests via diagnostics channels.Changes
Added event emission at key points in the HTTP client request lifecycle:
http.client.request.created: Emitted whenClientRequestis created (constructor completion)http.client.request.start: Emitted when the underlyingfetch()call startshttp.client.request.error: Emitted on request errorshttp.client.response.finish: Emitted when response headers are receivedImplementation Details
src/js/node/_http_client.ts{ request },{ request, error },{ request, response }response.finishfires when headers arrive, not when body completes (matching Node.js behavior despite the name)Testing
Bun-style tests
Added comprehensive tests in
test/js/node/diagnostics_channel/http-client-diagnostics.test.ts:http.get()convenience methodNode.js parallel tests
Added Node.js-style test in
test/js/node/test/parallel/test-diagnostics-channel-http-client.js:test-diagnostics-channel-http.jscommon.mustCall()assertionsFixes
Fixes getsentry/sentry-javascript#17779
This issue prevented Sentry's HTTP instrumentation from working in Bun, as it relies on these diagnostics_channel events to generate spans for HTTP requests.
Context
See also: Discord discussion https://discord.com/channels/876711213126520882/1423351347033407630
🤖 Generated with Claude Code