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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all \u003cpre\u003e\u003ccode\u003e blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks"); } } catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); } })(); (function(){ try { var __m = "github.com"; var __re = new RegExp('^' + "github\\.com" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length \u003e 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
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.

Sign up for GitHub

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
mydea merged 2 commits into from
Dec 13, 2024
Merged

feat(node)!: Avoid http spans by default for custom OTEL setups #14678

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b3660
feat(node)!: Avoid http spans by default for custom OTEL setups
mydea Dec 12, 2024
f0896c1
fix e2e tests
mydea Dec 12, 2024
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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-custom-sampler/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
1 change: 1 addition & 0 deletions dev-packages/e2e-tests/test-applications/node-otel-sdk-node/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
3 changes: 0 additions & 3 deletions dev-packages/e2e-tests/test-applications/node-otel-without-tracing/src/instrument.ts
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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.

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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."

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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?

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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 DownExpand 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 {

@lforstlforstDec 12, 2024

Copy link
Copy Markdown
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.

@mydeamydeaDec 12, 2024

Copy link
Copy Markdown
MemberAuthor

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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);
});
});
});