From d7dddbd91a5bdf0775c60a311d8b43882502acb5 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Mon, 23 Sep 2024 17:55:39 +0200 Subject: [PATCH 1/2] fix(core): Remove `sampled` flag from baggage meta tag in Tracing without Performance mode --- .../tracing/meta-tags-twp-errors/server.js | 32 ++++++++++++----- .../tracing/meta-tags-twp-errors/test.ts | 34 +++++++++++++++++-- .../suites/tracing/meta-tags-twp/test.ts | 1 + .../src/tracing/dynamicSamplingContext.ts | 8 ++++- .../tracing/dynamicSamplingContext.test.ts | 26 ++++++++++++-- 5 files changed, 85 insertions(+), 16 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js index 19877ffe3613..02f918b389cf 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js @@ -5,13 +5,15 @@ Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', transport: loggingTransport, beforeSend(event) { - event.contexts = { - ...event.contexts, - traceData: { - ...Sentry.getTraceData(), - metaTags: Sentry.getTraceMetaTags(), - }, - }; + if (!event.contexts.traceData) { + event.contexts = { + ...event.contexts, + traceData: { + ...Sentry.getTraceData(), + metaTags: Sentry.getTraceMetaTags(), + }, + }; + } return event; }, }); @@ -21,8 +23,20 @@ const express = require('express'); const app = express(); -app.get('/test', () => { - throw new Error('test error'); +app.get('/test', (_req, res) => { + Sentry.captureException(new Error('test error')); + res.status(200).send(); +}); + +app.get('/test-scope', (_req, res) => { + Sentry.withScope(scope => { + scope.setContext('traceData', { + ...Sentry.getTraceData(), + metaTags: Sentry.getTraceMetaTags(), + }); + Sentry.captureException(new Error('test error 2')); + }); + res.status(200).send(); }); Sentry.setupExpressErrorHandler(app); diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts index e6c0bfff822d..4bf454563509 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts @@ -5,7 +5,7 @@ describe('errors in TwP mode have same trace in trace context and getTraceData() cleanupChildProcesses(); }); - test('in incoming request', async () => { + test('in incoming request', done => { createRunner(__dirname, 'server.js') .expect({ event: event => { @@ -17,17 +17,44 @@ describe('errors in TwP mode have same trace in trace context and getTraceData() const traceData = contexts?.traceData || {}; expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`); + expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`); + expect(traceData.baggage).not.toContain('sentry-sampled='); expect(traceData.metaTags).toContain(``); - expect(traceData.metaTags).toContain(`sentr y-trace_id=${trace_id}`); + expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`); expect(traceData.metaTags).not.toContain('sentry-sampled='); }, }) - .start() + .start(done) .makeRequest('get', '/test'); }); + test('in incoming request with Custom Scope', done => { + 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.baggage).not.toContain('sentry-sampled='); + + expect(traceData.metaTags).toContain(``); + expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`); + expect(traceData.metaTags).not.toContain('sentry-sampled='); + }, + }) + .start(done) + .makeRequest('get', '/test-scope'); + }); + test('outside of a request handler', done => { createRunner(__dirname, 'no-server.js') .expect({ @@ -41,6 +68,7 @@ describe('errors in TwP mode have same trace in trace context and getTraceData() expect(traceData['sentry-trace']).toEqual(`${trace_id}-${span_id}`); expect(traceData.baggage).toContain(`sentry-trace_id=${trace_id}`); + expect(traceData.baggage).not.toContain('sentry-sampled='); expect(traceData.metaTags).toContain(``); expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`); diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp/test.ts index f3179beede6d..cca1f34321a2 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp/test.ts @@ -27,5 +27,6 @@ describe('getTraceMetaTags', () => { expect(sentryBaggageContent).toContain('sentry-environment=production'); expect(sentryBaggageContent).toContain('sentry-public_key=public'); expect(sentryBaggageContent).toContain(`sentry-trace_id=${traceId}`); + expect(sentryBaggageContent).not.toContain('sentry-sampled='); }); }); diff --git a/packages/core/src/tracing/dynamicSamplingContext.ts b/packages/core/src/tracing/dynamicSamplingContext.ts index d47dfd7ff317..d96dd726d51f 100644 --- a/packages/core/src/tracing/dynamicSamplingContext.ts +++ b/packages/core/src/tracing/dynamicSamplingContext.ts @@ -9,6 +9,7 @@ import { import { DEFAULT_ENVIRONMENT } from '../constants'; import { getClient } from '../currentScopes'; import { SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '../semanticAttributes'; +import { hasTracingEnabled } from '../utils/hasTracingEnabled'; import { getRootSpan, spanIsSampled, spanToJSON } from '../utils/spanUtils'; /** @@ -103,7 +104,12 @@ export function getDynamicSamplingContextFromSpan(span: Span): Readonly { environment: 'production', sampled: 'true', sample_rate: '0.56', - trace_id: expect.any(String), + trace_id: expect.stringMatching(/^[a-f0-9]{32}$/), transaction: 'tx', }); }); @@ -85,7 +86,7 @@ describe('getDynamicSamplingContextFromSpan', () => { environment: 'production', sampled: 'true', sample_rate: '1', - trace_id: expect.any(String), + trace_id: expect.stringMatching(/^[a-f0-9]{32}$/), transaction: 'tx', }); }); @@ -107,7 +108,7 @@ describe('getDynamicSamplingContextFromSpan', () => { environment: 'production', sampled: 'true', sample_rate: '0.56', - trace_id: expect.any(String), + trace_id: expect.stringMatching(/^[a-f0-9]{32}$/), transaction: 'tx', }); }); @@ -144,4 +145,23 @@ describe('getDynamicSamplingContextFromSpan', () => { expect(dsc.transaction).toEqual('tx'); }); }); + + it("doesn't return the sampled flag in the DSC if in Tracing without Performance mode", () => { + const rootSpan = new SentrySpan({ + name: 'tx', + sampled: undefined, + }); + + // Simulate TwP mode by deleting the tracesSampleRate option set in beforeEach + delete getClient()?.getOptions().tracesSampleRate; + + const dynamicSamplingContext = getDynamicSamplingContextFromSpan(rootSpan); + + expect(dynamicSamplingContext).toStrictEqual({ + release: '1.0.1', + environment: 'production', + trace_id: expect.stringMatching(/^[a-f0-9]{32}$/), + transaction: 'tx', + }); + }); }); From f7520f6688703109face2f98a48c919f38c7ff05 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 24 Sep 2024 13:31:29 +0200 Subject: [PATCH 2/2] cleanup tests --- .../tracing/meta-tags-twp-errors/server.js | 17 ------------- .../tracing/meta-tags-twp-errors/test.ts | 25 ------------------- 2 files changed, 42 deletions(-) diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js index 02f918b389cf..3b615e93cd16 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/server.js @@ -4,18 +4,6 @@ const Sentry = require('@sentry/node'); Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', transport: loggingTransport, - beforeSend(event) { - if (!event.contexts.traceData) { - event.contexts = { - ...event.contexts, - traceData: { - ...Sentry.getTraceData(), - metaTags: Sentry.getTraceMetaTags(), - }, - }; - } - return event; - }, }); // express must be required after Sentry is initialized @@ -24,11 +12,6 @@ const express = require('express'); const app = express(); app.get('/test', (_req, res) => { - Sentry.captureException(new Error('test error')); - res.status(200).send(); -}); - -app.get('/test-scope', (_req, res) => { Sentry.withScope(scope => { scope.setContext('traceData', { ...Sentry.getTraceData(), diff --git a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts index 4bf454563509..9abb7b1a631c 100644 --- a/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/meta-tags-twp-errors/test.ts @@ -30,31 +30,6 @@ describe('errors in TwP mode have same trace in trace context and getTraceData() .makeRequest('get', '/test'); }); - test('in incoming request with Custom Scope', done => { - 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.baggage).not.toContain('sentry-sampled='); - - expect(traceData.metaTags).toContain(``); - expect(traceData.metaTags).toContain(`sentry-trace_id=${trace_id}`); - expect(traceData.metaTags).not.toContain('sentry-sampled='); - }, - }) - .start(done) - .makeRequest('get', '/test-scope'); - }); - test('outside of a request handler', done => { createRunner(__dirname, 'no-server.js') .expect({