Skip to content

Commit

Permalink
fix(opentelemetry): Always use active span in Propagator.inject (#1…
Browse files Browse the repository at this point in the history
…3381)

This PR removes the check for `hasTracingEnabled` in
`getInjectionData` and simply always takes the non-recording span if
there is one. Which is always the case in server applications when handling a request.
For non-request-handling situations, we fall back anyway to the propagation context.
  • Loading branch information
Lms24 authored Sep 18, 2024
1 parent 479aa11 commit 14f0b6e
Show file tree
Hide file tree
Showing 4 changed files with 103 additions and 2 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
const { loggingTransport } = require('@sentry-internal/node-integration-tests');
const Sentry = require('@sentry/node');

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
release: '1.0',
transport: loggingTransport,
beforeSend(event) {
event.contexts = {
...event.contexts,
traceData: {
...Sentry.getTraceData(),
metaTags: Sentry.getTraceMetaTags(),
},
};
return event;
},
});

Sentry.captureException(new Error('test error'));
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
const { loggingTransport, startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');
const Sentry = require('@sentry/node');

Sentry.init({
dsn: 'https://public@dsn.ingest.sentry.io/1337',
transport: loggingTransport,
beforeSend(event) {
event.contexts = {
...event.contexts,
traceData: {
...Sentry.getTraceData(),
metaTags: Sentry.getTraceMetaTags(),
},
};
return event;
},
});

// express must be required after Sentry is initialized
const express = require('express');

const app = express();

app.get('/test', () => {
throw new Error('test error');
});

Sentry.setupExpressErrorHandler(app);

startExpressServerAndSendPortToRunner(app);
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
import { cleanupChildProcesses, createRunner } from '../../../utils/runner';

describe('errors in TwP mode have same trace in trace context and getTraceData()', () => {
afterAll(() => {
cleanupChildProcesses();
});

test('in incoming request', async () => {
createRunner(__dirname, 'server.js')
.expect({
event: event => {
const { contexts } = event;
const { trace_id, span_id } = contexts?.trace || {};
expect(trace_id).toMatch(/^[a-f0-9]{32}$/);
expect(span_id).toMatch(/^[a-f0-9]{16}$/);

const traceData = contexts?.traceData || {};

expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`);
expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`);

expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-${span_id}"/>`);
expect(traceData.metaTags).toContain(`sentr y-trace_id=${trace_id}`);
expect(traceData.metaTags).not.toContain('sentry-sampled=');
},
})
.start()
.makeRequest('get', '/test');
});

test('outside of a request handler', done => {
createRunner(__dirname, 'no-server.js')
.expect({
event: event => {
const { contexts } = event;
const { trace_id, span_id } = contexts?.trace || {};
expect(trace_id).toMatch(/^[a-f0-9]{32}$/);
expect(span_id).toMatch(/^[a-f0-9]{16}$/);

const traceData = contexts?.traceData || {};

expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`);
expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`);

expect(traceData.metaTags).toContain(`<meta name="sentry-trace" content="${trace_id}-${span_id}"/>`);
expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`);
expect(traceData.metaTags).not.toContain('sentry-sampled=');
},
})
.start(done);
});
});
3 changes: 1 addition & 2 deletions packages/opentelemetry/src/propagator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { propagation, trace } from '@opentelemetry/api';
import { W3CBaggagePropagator, isTracingSuppressed } from '@opentelemetry/core';
import { ATTR_URL_FULL, SEMATTRS_HTTP_URL } from '@opentelemetry/semantic-conventions';
import type { continueTrace } from '@sentry/core';
import { hasTracingEnabled } from '@sentry/core';
import { getRootSpan } from '@sentry/core';
import { spanToJSON } from '@sentry/core';
import {
Expand Down Expand Up @@ -198,7 +197,7 @@ function getInjectionData(context: Context): {
spanId: string | undefined;
sampled: boolean | undefined;
} {
const span = hasTracingEnabled() ? trace.getSpan(context) : undefined;
const span = trace.getSpan(context);
const spanIsRemote = span?.spanContext().isRemote;

// If we have a local span, we can just pick everything from it
Expand Down

0 comments on commit 14f0b6e

Please sign in to comment.