-
Notifications
You must be signed in to change notification settings - Fork 11
[#297] undici package instrumentation #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4221ee3 to
4f36515
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces instrumentation for the undici package while updating related test cases and the module hook configuration for improved compatibility with newer Node.js versions. Key changes include:
- Updating the conditional logic in bindHttpWithCallSite (agent-singleton-mock.js) to handle GrpcDataSender objects differently.
- Adding tests for undici instrumentation and updating mongodb endpoint assertions.
- Introducing a new gRPC data sender builder and extending instrumentation support in module-hook.js and the GitHub workflow.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/agent-singleton-mock.js | Revised bindHttpWithCallSite conditional logic |
| test/instrumentation/module/undici.test.js | New tests added to validate undici instrumentation |
| test/instrumentation/module/mongodb.test.js | Updated endpoint assertions using string splitting |
| test/client/grpc-data-sender-builder.js | New builder for configuring gRPC data sender settings |
| lib/instrumentation/module/undici.js | New undici instrumentation module with diagnostics channel |
| lib/instrumentation/module-hook.js | Added undici support to the module hook configuration |
| .github/workflows/main.yml | Extended Node.js version support by adding Node 22 |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
lib/instrumentation/module/undici.js:39
- [nitpick] The onMessage callback for the undici diagnostic channel only checks for a 'CONNECT' method and then returns without further handling; if this is a placeholder, adding a clarifying comment would improve maintainability.
this.subscribeDiagnosticsChannel('undici:request:create', ({ request }) =>{
8337e77 to
b116d2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds instrumentation support for the undici package and updates related HTTP tracing code. It also updates tests and configuration files to account for these changes.
- Updated instrumentation for undici with new diagnostics channels and global function initialization.
- Refactored usage of HttpOutgoingRequestHeader to HttpClientRequest across various modules.
- Updated test assertions and workflow configuration for additional Node.js versions.
Reviewed Changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/support/agent-singleton-mock.js | Refactored conditional branch when binding HTTP call sites. |
| test/instrumentation/request-header-utils.test.js | Updated tests to use the new HttpClientRequest. |
| test/instrumentation/module/undici.test.js | Added tests to verify undici instrumentation via diagnostics channels. |
| test/instrumentation/module/mongodb.test.js | Modified endpoint assertions to validate port extraction. |
| test/client/grpc-data-sender-builder.js | New builder for gRPC data sender with fluent API. |
| lib/instrumentation/module/undici.js | Implemented undici instrumentation based on version check and diagnostics channels. |
| lib/instrumentation/module-hook.js | Added global function loader for undici instrumentation. |
| lib/instrumentation/http/http-client-request.js | Refactored to remove HttpOutgoingRequestHeader and update getHostWithPathname method. |
| lib/instrumentation/http-shared.js | Updated to use HttpClientRequest for header manipulation and tracing. |
| lib/agent.js | Integrated global function loader for module hooks. |
| .github/workflows/main.yml | Expanded Node.js test matrix with Node 22. |
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
lib/instrumentation/http/http-client-request.js:48
- The removal of 'writeSampledHeaderFalse' and 'write' methods from HttpClientRequest appears to break the expected interface used in tracing (as seen in 'http-shared.js' and undici instrumentation). Consider reintroducing these methods with appropriate functionality to maintain compatibility with the rest of the instrumentation logic.
- writeSampledHeaderFalse() {
3cc4253 to
39a3d55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.
Files not reviewed (1)
- package.json: Language not supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 27 out of 28 changed files in this pull request and generated no comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (1)
test/client/client-side-stream-grpc-readable-stream.test.js:151
- Returning early without handling the case when expectedSpan is not found may allow the test to pass silently; consider adding an assertion to explicitly fail if expectedSpan is missing.
if (!expectedSpan) {
* Introduce fixture for isolated testing of gRPC stub methods in GrpcDataSenderBuilder * Disable undici Test on the Node.js v16 * Introduce Node.JS v22 test matrix * Remove HttpOutgoingRequestHeader cause by no needs wrapper class * undici's request differs from http.ClientRequest in header and host methods * feat(undici): ensure childTrace and recorder are passed via request object - Updated the implementation to align with the undici Diagnostics Channel API documentation. - Ensured that `childTraceBuilder` and `recorder` are consistently passed between channels (`undici:request:create`, `undici:request:headers`, `undici:request:trailers`, `undici:request:error`) using the `request` object. - This change improves trace context propagation and adheres to the documented behavior of the undici Diagnostics Channel API. - This change improves trace context propagation and adheres to the documented behavior of the undici Diagnostics Channel API. * Fix grpc pSpanEvent.setExceptioninfo error * Remove unused source: annotation.js, typed-value.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 26 out of 27 changed files in this pull request and generated no comments.
Files not reviewed (1)
- package.json: Language not supported
Comments suppressed due to low confidence (3)
test/client/grpc-data-sender.test.js:144
- The word 'applicaiton' appears to be misspelled; please correct it to 'application'.
t.equal(actual.getApplicationservicetype(), 1400, 'applicaiton service type')
test/client/grpc-data-sender.test.js:153
- Removal of server.forceShutdown() in the teardown could lead to lingering server processes; ensure that the server is properly shut down elsewhere.
t.teardown(() => { dataSender.close() server.forceShutdown() }) removed
test/client/grpc-data-sender.test.js:285
- Changing the expected span event index from 0 to a dynamic index may cause mismatches if only one event is expected; please verify that this change aligns with the intended behavior.
const expectedSpanEvent = expectedSpanChunk.spanEventList[index]
No description provided.