Support undici via diagnostics_channel events #7206
Conversation
This provides now complete support for undici, not just fetch only.
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
This is mostly looking very good! I just left some small improvement requests :)
| const HTTP_STATUS_CODE = tags.HTTP_STATUS_CODE | ||
| const HTTP_REQUEST_HEADERS = tags.HTTP_REQUEST_HEADERS | ||
| const HTTP_RESPONSE_HEADERS = tags.HTTP_RESPONSE_HEADERS |
There was a problem hiding this comment.
Nit
| const HTTP_STATUS_CODE = tags.HTTP_STATUS_CODE | |
| const HTTP_REQUEST_HEADERS = tags.HTTP_REQUEST_HEADERS | |
| const HTTP_RESPONSE_HEADERS = tags.HTTP_RESPONSE_HEADERS | |
| const { | |
| HTTP_STATUS_CODE, | |
| HTTP_REQUEST_HEADERS, | |
| HTTP_RESPONSE_HEADERS, | |
| } = tags |
| // These fire for undici >= 4.7.0 for ALL request types (fetch, request, stream, etc.) | ||
| // =========================================== | ||
|
|
||
| _onNativeRequestCreate ({ request }) { |
There was a problem hiding this comment.
Please use private properties (also for the other methods)
| const origin = request.origin || '' | ||
| const path = request.path || '/' | ||
| const method = (request.method || 'GET').toUpperCase() |
There was a problem hiding this comment.
| const origin = request.origin || '' | |
| const path = request.path || '/' | |
| const method = (request.method || 'GET').toUpperCase() | |
| const { origin = '', path = '/' } = request | |
| const method = request.method?.toUpperCase() ?? 'GET' |
| } | ||
|
|
||
| // Store span context on request for later retrieval | ||
| request[kSpanContext] = { |
There was a problem hiding this comment.
Could we just use a WeakMap instead? That would prevent the need for the manipulation
| if (response?.headers && this.config.headers) { | ||
| const headers = normalizeHeaders(response.headers) | ||
|
|
||
| for (const [key, tag] of this.config.headers) { | ||
| const value = headers[key] | ||
| if (value) { | ||
| span.setTag(tag || `${HTTP_RESPONSE_HEADERS}.${key}`, value) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we just use a small utility function that works for response and requests? That way there is less code duplication :)
| const store = super.bindStart(ctx) | ||
|
|
||
| // Inject trace headers back into the request | ||
| for (const name in options.headers) { |
There was a problem hiding this comment.
Nit
| for (const name in options.headers) { | |
| for (const name of Object.keys(options.headers)) { |
| if (ctx.error && ctx.error.name === 'AbortError') return | ||
| return super.error(ctx) |
There was a problem hiding this comment.
Nit
| if (ctx.error && ctx.error.name === 'AbortError') return | |
| return super.error(ctx) | |
| if (!ctx.error || ctx.error.name !== 'AbortError') { | |
| return super.error(ctx) | |
| } |
| assert.strictEqual(traces[0][0].meta[ERROR_MESSAGE], error.message || error.code) | ||
| assert.strictEqual(traces[0][0].meta[ERROR_STACK], error.stack) | ||
| assert.strictEqual(traces[0][0].meta.component, 'undici') | ||
| assert.strictEqual(traces[0][0].error, 1) |
There was a problem hiding this comment.
Nit while being on it: could you rewrite these to use assertObjectContains(traces, [[{ meta: { ... } }]]) instead?
| assert.strictEqual(traces[0][0].meta['http.method'], 'GET') | ||
| assert.strictEqual(traces[0][0].meta['http.status_code'], '200') | ||
| assert.strictEqual(traces[0][0].meta.component, 'undici') | ||
| assert.strictEqual(traces[0][0].meta['out.host'], 'localhost') |
There was a problem hiding this comment.
Please use assertObjectContains() instead.
| }) | ||
| appListener = server(app, port => { | ||
| agent | ||
| .assertSomeTraces(traces => { |
There was a problem hiding this comment.
Please use agent.assertFirstTrace({ resource: 'POST', ... })
The same with other tests
2 participants
What does this PR do?
This provides now complete support for undici, not just fetch only.
Motivation
Presently the Undici instrumentation only supports the
fetch(...)function, not the rest of the library. This adds full support via the diagnostics_channel events.Plugin Checklist
Additional Notes