-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(node): Ensure traces are propagated without spans in Node 23+ #16221
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
Open
mydea
wants to merge
3
commits into
develop
Choose a base branch
from
fn/propagate-traces-node-http
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
17 changes: 17 additions & 0 deletions
17
...es/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/instrument.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
import * as Sentry from '@sentry/node'; | ||
import { loggingTransport } from '@sentry-internal/node-integration-tests'; | ||
|
||
Sentry.init({ | ||
dsn: 'https://[email protected]/1337', | ||
release: '1.0', | ||
tracePropagationTargets: [/\/v0/, 'v1'], | ||
integrations: [Sentry.httpIntegration({ spans: false })], | ||
transport: loggingTransport, | ||
// Ensure this gets a correct hint | ||
beforeBreadcrumb(breadcrumb, hint) { | ||
breadcrumb.data = breadcrumb.data || {}; | ||
const req = hint?.request; | ||
breadcrumb.data.ADDED_PATH = req?.path; | ||
return breadcrumb; | ||
}, | ||
}); |
43 changes: 43 additions & 0 deletions
43
...ages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.mjs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,43 @@ | ||
import * as Sentry from '@sentry/node'; | ||
import * as http from 'http'; | ||
|
||
async function run() { | ||
Sentry.addBreadcrumb({ message: 'manual breadcrumb' }); | ||
|
||
await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`); | ||
await makeHttpGet(`${process.env.SERVER_URL}/api/v1`); | ||
await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`); | ||
await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`); | ||
|
||
Sentry.captureException(new Error('foo')); | ||
} | ||
|
||
run(); | ||
|
||
function makeHttpRequest(url) { | ||
return new Promise(resolve => { | ||
http | ||
.request(url, httpRes => { | ||
httpRes.on('data', () => { | ||
// we don't care about data | ||
}); | ||
httpRes.on('end', () => { | ||
resolve(); | ||
}); | ||
}) | ||
.end(); | ||
}); | ||
} | ||
|
||
function makeHttpGet(url) { | ||
return new Promise(resolve => { | ||
http.get(url, httpRes => { | ||
httpRes.on('data', () => { | ||
// we don't care about data | ||
}); | ||
httpRes.on('end', () => { | ||
resolve(); | ||
}); | ||
}); | ||
}); | ||
} |
204 changes: 204 additions & 0 deletions
204
dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,204 @@ | ||
import { describe, expect } from 'vitest'; | ||
import { conditionalTest } from '../../../../utils'; | ||
import { createEsmAndCjsTests } from '../../../../utils/runner'; | ||
import { createTestServer } from '../../../../utils/server'; | ||
|
||
describe('outgoing http requests with tracing & spans disabled', () => { | ||
createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { | ||
conditionalTest({ min: 23 })('node >=23', () => { | ||
test('outgoing http requests are correctly instrumented with tracing & spans disabled', async () => { | ||
expect.assertions(11); | ||
|
||
const [SERVER_URL, closeTestServer] = await createTestServer() | ||
.get('/api/v0', headers => { | ||
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); | ||
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); | ||
expect(headers['baggage']).toEqual(expect.any(String)); | ||
}) | ||
.get('/api/v1', headers => { | ||
expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/)); | ||
expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000'); | ||
expect(headers['baggage']).toEqual(expect.any(String)); | ||
}) | ||
.get('/api/v2', headers => { | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.get('/api/v3', headers => { | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.start(); | ||
|
||
await createRunner() | ||
.withEnv({ SERVER_URL }) | ||
.ensureNoErrorOutput() | ||
.expect({ | ||
event: { | ||
exception: { | ||
values: [ | ||
{ | ||
type: 'Error', | ||
value: 'foo', | ||
}, | ||
], | ||
}, | ||
breadcrumbs: [ | ||
{ | ||
message: 'manual breadcrumb', | ||
timestamp: expect.any(Number), | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v0`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v0', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v1`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v1', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v2`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v2', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v3`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v3', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
], | ||
}, | ||
}) | ||
.start() | ||
.completed(); | ||
|
||
closeTestServer(); | ||
}); | ||
}); | ||
|
||
// On older node versions, outgoing requests do not get trace-headers injected, sadly | ||
// This is because the necessary diagnostics channel hook is not available yet | ||
conditionalTest({ max: 22 })('node <=22', () => { | ||
test('outgoing http requests generate breadcrumbs correctly with tracing & spans disabled', async () => { | ||
expect.assertions(9); | ||
|
||
const [SERVER_URL, closeTestServer] = await createTestServer() | ||
.get('/api/v0', headers => { | ||
// This is not instrumented, sadly | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.get('/api/v1', headers => { | ||
// This is not instrumented, sadly | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.get('/api/v2', headers => { | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.get('/api/v3', headers => { | ||
expect(headers['baggage']).toBeUndefined(); | ||
expect(headers['sentry-trace']).toBeUndefined(); | ||
}) | ||
.start(); | ||
|
||
await createRunner() | ||
.withEnv({ SERVER_URL }) | ||
.ensureNoErrorOutput() | ||
.expect({ | ||
event: { | ||
exception: { | ||
values: [ | ||
{ | ||
type: 'Error', | ||
value: 'foo', | ||
}, | ||
], | ||
}, | ||
breadcrumbs: [ | ||
{ | ||
message: 'manual breadcrumb', | ||
timestamp: expect.any(Number), | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v0`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v0', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v1`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v1', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v2`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v2', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
{ | ||
category: 'http', | ||
data: { | ||
'http.method': 'GET', | ||
url: `${SERVER_URL}/api/v3`, | ||
status_code: 200, | ||
ADDED_PATH: '/api/v3', | ||
}, | ||
timestamp: expect.any(Number), | ||
type: 'http', | ||
}, | ||
], | ||
}, | ||
}) | ||
.start() | ||
.completed(); | ||
|
||
closeTestServer(); | ||
}); | ||
}); | ||
}); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,13 +18,17 @@ import { | |||||||||||
getCurrentScope, | ||||||||||||
getIsolationScope, | ||||||||||||
getSanitizedUrlString, | ||||||||||||
getTraceData, | ||||||||||||
httpRequestToRequestData, | ||||||||||||
logger, | ||||||||||||
LRUMap, | ||||||||||||
parseUrl, | ||||||||||||
stripUrlQueryAndFragment, | ||||||||||||
withIsolationScope, | ||||||||||||
} from '@sentry/core'; | ||||||||||||
import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry'; | ||||||||||||
import { DEBUG_BUILD } from '../../debug-build'; | ||||||||||||
import { mergeBaggageHeaders } from '../../utils/baggage'; | ||||||||||||
import { getRequestUrl } from '../../utils/getRequestUrl'; | ||||||||||||
|
||||||||||||
const INSTRUMENTATION_NAME = '@sentry/instrumentation-http'; | ||||||||||||
|
@@ -49,6 +53,15 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & { | |||||||||||
*/ | ||||||||||||
extractIncomingTraceFromHeader?: boolean; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Whether to propagate Sentry trace headers in outgoing requests. | ||||||||||||
* By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled) | ||||||||||||
* then this instrumentation can take over. | ||||||||||||
* | ||||||||||||
* @default `false` | ||||||||||||
*/ | ||||||||||||
propagateTraceInOutgoingRequests?: boolean; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Do not capture breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`. | ||||||||||||
* For the scope of this instrumentation, this callback only controls breadcrumb creation. | ||||||||||||
|
@@ -102,8 +115,12 @@ const MAX_BODY_BYTE_LENGTH = 1024 * 1024; | |||||||||||
* https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts | ||||||||||||
*/ | ||||||||||||
export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpInstrumentationOptions> { | ||||||||||||
private _propagationDecisionMap: LRUMap<string, boolean>; | ||||||||||||
|
||||||||||||
public constructor(config: SentryHttpInstrumentationOptions = {}) { | ||||||||||||
super(INSTRUMENTATION_NAME, VERSION, config); | ||||||||||||
|
||||||||||||
this._propagationDecisionMap = new LRUMap<string, boolean>(100); | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** @inheritdoc */ | ||||||||||||
|
@@ -127,6 +144,11 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |||||||||||
this._onOutgoingRequestFinish(data.request, undefined); | ||||||||||||
}) satisfies ChannelListener; | ||||||||||||
|
||||||||||||
const onHttpClientRequestCreated = ((_data: unknown) => { | ||||||||||||
const data = _data as { request: http.ClientRequest }; | ||||||||||||
this._onOutgoingRequestCreated(data.request); | ||||||||||||
}) satisfies ChannelListener; | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* You may be wondering why we register these diagnostics-channel listeners | ||||||||||||
* in such a convoluted way (as InstrumentationNodeModuleDefinition...)˝, | ||||||||||||
|
@@ -153,12 +175,19 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |||||||||||
// In this case, `http.client.response.finish` is not triggered | ||||||||||||
subscribe('http.client.request.error', onHttpClientRequestError); | ||||||||||||
|
||||||||||||
// NOTE: This channel only exist since Node 23 | ||||||||||||
// Before that, outgoing requests are not patched, sadly | ||||||||||||
Comment on lines
+178
to
+179
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||
if (this.getConfig().propagateTraceInOutgoingRequests) { | ||||||||||||
subscribe('http.client.request.created', onHttpClientRequestCreated); | ||||||||||||
} | ||||||||||||
|
||||||||||||
return moduleExports; | ||||||||||||
}, | ||||||||||||
() => { | ||||||||||||
unsubscribe('http.server.request.start', onHttpServerRequestStart); | ||||||||||||
unsubscribe('http.client.response.finish', onHttpClientResponseFinish); | ||||||||||||
unsubscribe('http.client.request.error', onHttpClientRequestError); | ||||||||||||
unsubscribe('http.client.request.created', onHttpClientRequestCreated); | ||||||||||||
}, | ||||||||||||
), | ||||||||||||
new InstrumentationNodeModuleDefinition( | ||||||||||||
|
@@ -209,6 +238,49 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns | |||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* This is triggered when an outgoing request is created. | ||||||||||||
* It has access to the request object, and can mutate it before the request is sent. | ||||||||||||
*/ | ||||||||||||
private _onOutgoingRequestCreated(request: http.ClientRequest): void { | ||||||||||||
const url = getRequestUrl(request); | ||||||||||||
const ignoreOutgoingRequests = this.getConfig().ignoreOutgoingRequests; | ||||||||||||
const shouldPropagate = | ||||||||||||
typeof ignoreOutgoingRequests === 'function' ? !ignoreOutgoingRequests(url, getRequestOptions(request)) : true; | ||||||||||||
|
||||||||||||
if (!shouldPropagate) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
// Manually add the trace headers, if it applies | ||||||||||||
// Note: We do not use `propagation.inject()` here, because our propagator relies on an active span | ||||||||||||
// Which we do not have in this case | ||||||||||||
const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets; | ||||||||||||
const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, this._propagationDecisionMap) | ||||||||||||
? getTraceData() | ||||||||||||
: undefined; | ||||||||||||
|
||||||||||||
if (!addedHeaders) { | ||||||||||||
return; | ||||||||||||
} | ||||||||||||
|
||||||||||||
const { 'sentry-trace': sentryTrace, baggage } = addedHeaders; | ||||||||||||
|
||||||||||||
// We do not want to overwrite existing header here, if it was already set | ||||||||||||
if (sentryTrace && !request.getHeader('sentry-trace')) { | ||||||||||||
request.setHeader('sentry-trace', sentryTrace); | ||||||||||||
logger.log(INSTRUMENTATION_NAME, 'Added sentry-trace header to outgoing request'); | ||||||||||||
} | ||||||||||||
|
||||||||||||
if (baggage) { | ||||||||||||
// For baggage, we make sure to merge this into a possibly existing header | ||||||||||||
const newBaggage = mergeBaggageHeaders(request.getHeader('baggage'), baggage); | ||||||||||||
if (newBaggage) { | ||||||||||||
request.setHeader('baggage', newBaggage); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
/** | ||||||||||||
* Patch a server.emit function to handle process isolation for incoming requests. | ||||||||||||
* This will only patch the emit function if it was not already patched. | ||||||||||||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
q: Should we mention that this only works for Node >= 23?