Skip to content

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

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ Sentry.init({
skipOpenTelemetrySetup: true,
// By defining _any_ sample rate, tracing integrations will be added by default
tracesSampleRate: 0,
integrations: [Sentry.httpIntegration({ spans: true })],
});

const provider = new NodeTracerProvider({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ const sentryClient = Sentry.init({
tracesSampleRate: 1,

skipOpenTelemetrySetup: true,
integrations: [Sentry.httpIntegration({ spans: true })],
});

const sdk = new opentelemetry.NodeSDK({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,6 @@ Sentry.init({
debug: !!process.env.DEBUG,
tunnel: `http://localhost:3031/`, // proxy server
// Tracing is completely disabled

integrations: [Sentry.httpIntegration({ spans: false })],

// Custom OTEL setup
skipOpenTelemetrySetup: true,
});
Expand Down
1 change: 1 addition & 0 deletions docs/migration/draft-v9-migration-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,3 +158,4 @@
- Deprecated `addOpenTelemetryInstrumentation`. Use the `openTelemetryInstrumentations` option in `Sentry.init()` or your custom Sentry Client instead.
- Deprecated `registerEsmLoaderHooks.include` and `registerEsmLoaderHooks.exclude`. Set `onlyIncludeInstrumentedModules: true` instead.
- `registerEsmLoaderHooks` will only accept `true | false | undefined` in the future. The SDK will default to wrapping modules that are used as part of OpenTelemetry Instrumentation.
- `httpIntegration({ spans: false })` is configured by default if `skipOpenTelemetrySetup: true` is set. You can still overwrite this if desired.
Copy link
Contributor

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."

Copy link
Member Author

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?

Copy link
Contributor

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.

6 changes: 6 additions & 0 deletions docs/migration/v8-to-v9.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ If you need to support older browsers, we recommend transpiling your code using

## 2. Behavior Changes

### `@sentry/node`

- When `skipOpenTelemetrySetup: true` is configured, `httpIntegration({ spans: false })` will be configured by default. This means that you no longer have to specify this yourself in this scenario. With this change, no spans are emitted once `skipOpenTelemetrySetup: true` is configured, without any further configuration being needed.

### Uncategorized (TODO)

- Next.js withSentryConfig returning Promise
- `request` on sdk processing metadata will be ignored going forward
- respect sourcemap generation settings
Expand Down
14 changes: 13 additions & 1 deletion packages/node/src/integrations/http/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;

Expand Down Expand Up @@ -118,12 +121,21 @@ export const instrumentOtelHttp = generateInstrumentOnce<HttpInstrumentationConf
return instrumentation;
});

/** Exported only for tests. */
export function _shouldInstrumentSpans(options: HttpOptions, clientOptions: Partial<NodeClientOptions> = {}): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ! somewhere everything can become wrong, which can be easy to miss when testing "happy paths" in integration tests only.

// 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);
}
Expand Down
18 changes: 18 additions & 0 deletions packages/node/test/integrations/http.test.ts
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);
});
});
});
Loading