-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(node)!: Avoid http spans by default for custom OTEL setups #14678
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { getClient } from '@sentry/opentelemetry'; | |
import { generateInstrumentOnce } from '../../otel/instrument'; | ||
import type { NodeClient } from '../../sdk/client'; | ||
import type { HTTPModuleRequestIncomingMessage } from '../../transports/http-module'; | ||
import type { NodeClientOptions } from '../../types'; | ||
import { addOriginToSpan } from '../../utils/addOriginToSpan'; | ||
import { getRequestUrl } from '../../utils/getRequestUrl'; | ||
import { SentryHttpInstrumentation } from './SentryHttpInstrumentation'; | ||
|
@@ -27,6 +28,8 @@ interface HttpOptions { | |
* If set to false, do not emit any spans. | ||
* This will ensure that the default HttpInstrumentation from OpenTelemetry is not setup, | ||
* only the Sentry-specific instrumentation for request isolation is applied. | ||
* | ||
* If `skipOpenTelemetrySetup: true` is configured, this defaults to `false`, otherwise it defaults to `true`. | ||
*/ | ||
spans?: boolean; | ||
|
||
|
@@ -118,12 +121,21 @@ export const instrumentOtelHttp = generateInstrumentOnce<HttpInstrumentationConf | |
return instrumentation; | ||
}); | ||
|
||
/** Exported only for tests. */ | ||
export function _shouldInstrumentSpans(options: HttpOptions, clientOptions: Partial<NodeClientOptions> = {}): boolean { | ||
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. l: Do we really need a helper function + test for this? Imo could just be a const with description. Makes it rather hard to read. 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. I just wanted to double check the behavior is correct without adding integration tests etc for this 😅 I always find it easy for little bugs to sneak in in such places, e.g. if you have a wrong |
||
// If `spans` is passed in, it takes precedence | ||
// Else, we by default emit spans, unless `skipOpenTelemetrySetup` is set to `true` | ||
return typeof options.spans === 'boolean' ? options.spans : !clientOptions.skipOpenTelemetrySetup; | ||
} | ||
|
||
/** | ||
* Instrument the HTTP and HTTPS modules. | ||
*/ | ||
const instrumentHttp = (options: HttpOptions = {}): void => { | ||
const instrumentSpans = _shouldInstrumentSpans(options, getClient<NodeClient>()?.getOptions()); | ||
|
||
// This is the "regular" OTEL instrumentation that emits spans | ||
if (options.spans !== false) { | ||
if (instrumentSpans) { | ||
const instrumentationConfig = getConfigWithDefaults(options); | ||
instrumentOtelHttp(instrumentationConfig); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { _shouldInstrumentSpans } from '../../src/integrations/http'; | ||
|
||
describe('httpIntegration', () => { | ||
describe('_shouldInstrumentSpans', () => { | ||
it.each([ | ||
[{}, {}, true], | ||
[{ spans: true }, {}, true], | ||
[{ spans: false }, {}, false], | ||
[{ spans: true }, { skipOpenTelemetrySetup: true }, true], | ||
[{ spans: false }, { skipOpenTelemetrySetup: true }, false], | ||
[{}, { skipOpenTelemetrySetup: true }, false], | ||
[{}, { skipOpenTelemetrySetup: false }, true], | ||
])('returns the correct value for options=%p and clientOptions=%p', (options, clientOptions, expected) => { | ||
const actual = _shouldInstrumentSpans(options, clientOptions); | ||
expect(actual).toBe(expected); | ||
}); | ||
}); | ||
}); |
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.
I would phrase it a bit differently and explain it to people like this: "The httpIntegration will no longer set up the Opentelemetry HTTP Instrumentation when skipOpenTelemetrySetup: true, or when spans: false is set."
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.
I added "better" docs to the v8-to-v9 migration guide, which I only saw after I wrote this 😅 should we maintain both of these docs?
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.
I will merge them into one once develop becomes the "main" branch for the v9 work.