Skip to content

Commit

Permalink
feat(node): Implement Sentry-specific http instrumentation (#13763)
Browse files Browse the repository at this point in the history
This PR is a pretty big change, but it should help us to make custom
OTEL support way better/easier to understand in the future.

The fundamental problem this PR is trying to change is the restriction
that we rely on our instance of `HttpInstrumentation` for
Sentry-specific (read: not span-related) things. This made it
tricky/annoying for users with a custom OTEL setup that may include
`HttpInstrumentation` to get things working, because they may
inadvertedly overwrite our instance of the instrumentation (because
there can only be a single monkey-patch per module in the regular
instrumentations), leading to hard-to-debug and often subtle problems.

This PR fixes this by splitting out the non-span related http
instrumentation code into a new, dedicated `SentryHttpInstrumentation`,
which can be run side-by-side with the OTEL instrumentation (which emits
spans, ...).

We make this work by basically implementing our own custom, minimal
`wrap` method instead of using shimmer. This way, OTEL instrumentation
cannot identify the wrapped module as wrapped, and allow to wrap it
again. While this is _slightly_ hacky and also means you cannot unwrap
the http module, we do not generally support this for the Sentry SDK
anyhow.

This new Instrumentation does two things:

1. Handles automatic forking of the isolation scope per incoming
request. By using our own code, we can actually make this much nicer, as
we do not need to retrospectively update the isolation scope anymore,
but instead we can do this properly now.
2. Emit breadcrumbs for outgoing requests.

With this change, in errors only mode you really do not need our
instance of the default `HttpInstrumentation` anymore at all, you
can/should just provide your own if you want to capture http spans in a
non-Sentry environment. However, this is sadly a bit tricky, because up
to now we forced users in this scenario to still use our Http instance
and avoid adding their own (instead we allowed users to pass their Http
instrumentation config to our Http integration). This means that if we'd
simply stop adding our http instrumentation instance when tracing is
disabled, these users would stop getting otel spans as well :/ so we
sadly can't change this without a major.

Instead, I re-introduced the `spans: false` for `httpIntegration({
spans: false })`. When this is set (which for now is opt-in, but
probably should be opt-out in v9) we will only register
SentryHttpInstrumentation, not HttpInstrumentation, thus not emitting
any spans. Users can add their own instance of HttpInstrumentation if
they care.

One semi-related thing that I noticed while looking into this is that we
incorrectly emitted node-fetch spans in errors-only mode. This
apparently sneaked in when we migrated to the new undici
instrumentation. I extracted this out into a dedicated PR too, but the
changes are in this too because tests were a bit fucked up otherwise.

On top of #13765

This also includes a bump of import-in-the-middle to 1.11.2, as this
includes a fix to properly allow double-wrapping ESM modules.
  • Loading branch information
mydea authored and billyvg committed Oct 17, 2024
1 parent 89d748b commit 0248545
Show file tree
Hide file tree
Showing 16 changed files with 557 additions and 409 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
},
"devDependencies": {
"@sentry-internal/test-utils": "link:../../../test-utils",
"@playwright/test": "^1.44.1",
"wait-port": "1.0.4"
"@playwright/test": "^1.44.1"
},
"volta": {
"extends": "../../package.json"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,7 @@
},
"devDependencies": {
"@sentry-internal/test-utils": "link:../../../test-utils",
"@playwright/test": "^1.41.1",
"wait-port": "1.0.4"
"@playwright/test": "^1.41.1"
},
"volta": {
"extends": "../../package.json"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ test('AWS Serverless SDK sends events in ESM mode', async ({ request }) => {
);

child_process.execSync('pnpm start', {
stdio: 'ignore',
stdio: 'inherit',
});

const transactionEvent = await transactionEventPromise;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
"@opentelemetry/sdk-trace-node": "1.26.0",
"@opentelemetry/exporter-trace-otlp-http": "0.53.0",
"@opentelemetry/instrumentation-undici": "0.6.0",
"@opentelemetry/instrumentation-http": "0.53.0",
"@opentelemetry/instrumentation": "0.53.0",
"@sentry/core": "latest || *",
"@sentry/node": "latest || *",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import express from 'express';
const app = express();
const port = 3030;

Sentry.setTag('root-level-tag', 'yes');

app.get('/test-success', function (req, res) {
res.send({ version: 'v1' });
});
Expand All @@ -23,8 +25,6 @@ app.get('/test-transaction', function (req, res) {

await fetch('http://localhost:3030/test-success');

await Sentry.flush();

res.send({});
});
});
Expand All @@ -38,7 +38,10 @@ app.get('/test-error', async function (req, res) {
});

app.get('/test-exception/:id', function (req, _res) {
throw new Error(`This is an exception with id ${req.params.id}`);
const id = req.params.id;
Sentry.setTag(`param-${id}`, id);

throw new Error(`This is an exception with id ${id}`);
});

Sentry.setupExpressErrorHandler(app);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
const { HttpInstrumentation } = require('@opentelemetry/instrumentation-http');
const { NodeTracerProvider, BatchSpanProcessor } = require('@opentelemetry/sdk-trace-node');
const { OTLPTraceExporter } = require('@opentelemetry/exporter-trace-otlp-http');
const Sentry = require('@sentry/node');
const { SentrySpanProcessor, SentryPropagator } = require('@sentry/opentelemetry');
const { SentryPropagator } = require('@sentry/opentelemetry');
const { UndiciInstrumentation } = require('@opentelemetry/instrumentation-undici');
const { registerInstrumentations } = require('@opentelemetry/instrumentation');

Expand All @@ -15,6 +16,8 @@ Sentry.init({
tunnel: `http://localhost:3031/`, // proxy server
// Tracing is completely disabled

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

// Custom OTEL setup
skipOpenTelemetrySetup: true,
});
Expand All @@ -37,5 +40,5 @@ provider.register({
});

registerInstrumentations({
instrumentations: [new UndiciInstrumentation()],
instrumentations: [new UndiciInstrumentation(), new HttpInstrumentation()],
});
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,24 @@ test('Sends correct error event', async ({ baseURL }) => {
span_id: expect.any(String),
});
});

test('Isolates requests correctly', async ({ baseURL }) => {
const errorEventPromise1 = waitForError('node-otel-without-tracing', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 555-a';
});
const errorEventPromise2 = waitForError('node-otel-without-tracing', event => {
return !event.type && event.exception?.values?.[0]?.value === 'This is an exception with id 555-b';
});

fetch(`${baseURL}/test-exception/555-a`);
fetch(`${baseURL}/test-exception/555-b`);

const errorEvent1 = await errorEventPromise1;
const errorEvent2 = await errorEventPromise2;

expect(errorEvent1.transaction).toEqual('GET /test-exception/555-a');
expect(errorEvent1.tags).toEqual({ 'root-level-tag': 'yes', 'param-555-a': '555-a' });

expect(errorEvent2.transaction).toEqual('GET /test-exception/555-b');
expect(errorEvent2.tags).toEqual({ 'root-level-tag': 'yes', 'param-555-b': '555-b' });
});
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,7 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {

const scopeSpans = json.resourceSpans?.[0]?.scopeSpans;

const httpScope = scopeSpans?.find(
scopeSpan => scopeSpan.scope.name === '@opentelemetry_sentry-patched/instrumentation-http',
);
const httpScope = scopeSpans?.find(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http');

return (
httpScope &&
Expand All @@ -40,9 +38,7 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
// But our default node-fetch spans are not emitted
expect(scopeSpans.length).toEqual(2);

const httpScopes = scopeSpans?.filter(
scopeSpan => scopeSpan.scope.name === '@opentelemetry_sentry-patched/instrumentation-http',
);
const httpScopes = scopeSpans?.filter(scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-http');
const undiciScopes = scopeSpans?.filter(
scopeSpan => scopeSpan.scope.name === '@opentelemetry/instrumentation-undici',
);
Expand Down Expand Up @@ -114,7 +110,6 @@ test('Sends an API route transaction to OTLP', async ({ baseURL }) => {
{ key: 'net.peer.port', value: { intValue: expect.any(Number) } },
{ key: 'http.status_code', value: { intValue: 200 } },
{ key: 'http.status_text', value: { stringValue: 'OK' } },
{ key: 'sentry.origin', value: { stringValue: 'auto.http.otel.http' } },
]),
droppedAttributesCount: 0,
events: [],
Expand Down
2 changes: 1 addition & 1 deletion packages/node/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@
"@sentry/opentelemetry": "8.34.0",
"@sentry/types": "8.34.0",
"@sentry/utils": "8.34.0",
"import-in-the-middle": "^1.11.0"
"import-in-the-middle": "^1.11.2"
},
"devDependencies": {
"@types/node": "^14.18.0"
Expand Down
Loading

0 comments on commit 0248545

Please sign in to comment.