Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Streamline sentry-trace, baggage and DSC handling #14364

Merged
merged 36 commits into from
Nov 26, 2024
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
464d725
ref: Streamline sentry-trace, baggage and DSC handling
mydea Nov 19, 2024
b31203e
move files around
mydea Nov 19, 2024
07c8fcf
streamline propagator
mydea Nov 19, 2024
81d87ff
ref: Rename & streamline
mydea Nov 19, 2024
8bd3ef8
better comments & small ref
mydea Nov 19, 2024
5201fc8
fix tests
mydea Nov 19, 2024
1608c4c
ref: Use `getTraceData` instead
mydea Nov 20, 2024
f6208e0
fix tests & checks
mydea Nov 20, 2024
04227f2
fix linting
mydea Nov 20, 2024
a5f8c1e
"fix" remix test???
mydea Nov 20, 2024
12c9f45
revert
mydea Nov 20, 2024
e2de55d
update test??
mydea Nov 20, 2024
d0c92b8
fix remix instrument server
mydea Nov 20, 2024
112ca89
unflake unrelated test
mydea Nov 20, 2024
c47f0ac
minimal changes
mydea Nov 20, 2024
58bb2ac
fix propagator!!
mydea Nov 20, 2024
ec4aa58
Revert "fix propagator!!"
mydea Nov 20, 2024
ad67471
WIP do not use custom getTraceData in OTEL???
mydea Nov 20, 2024
7b423e8
ref: Use `getInjectionData` for otel-specific `getTraceData`
mydea Nov 20, 2024
7a557f4
Merge branch 'develop' into fn/wip3
mydea Nov 21, 2024
e026314
fix imports after merge
mydea Nov 21, 2024
7d5865e
bump size-limit
mydea Nov 21, 2024
0d5c32d
Merge branch 'develop' into fn/headers-unify
mydea Nov 21, 2024
f8a5d82
fix linting
mydea Nov 21, 2024
a2e4c8f
small size improvement
mydea Nov 22, 2024
d43ceae
improve fetch bundle size slightly
mydea Nov 22, 2024
b8432f9
Merge branch 'develop' into fn/headers-unify
mydea Nov 22, 2024
5ae3b58
Merge branch 'develop' into fn/headers-unify
mydea Nov 22, 2024
aa3846a
fixes for tests
mydea Nov 22, 2024
ae0e8ec
Merge branch 'develop' into fn/headers-unify
mydea Nov 25, 2024
9f0b1ea
update size limits :(
mydea Nov 25, 2024
d5f08b6
Merge branch 'develop' into fn/headers-unify
mydea Nov 26, 2024
a20fc43
fix linting
mydea Nov 26, 2024
c3963be
fix merge, oops
mydea Nov 26, 2024
57bfd0e
fix fetch
mydea Nov 26, 2024
80ecf6b
fix fetch order
mydea Nov 26, 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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ sentryTest('should not add source context lines to errors from script files', as
const url = await getLocalTestUrl({ testDir: __dirname });

const eventReqPromise = waitForErrorRequestOnUrl(page, url);
await page.waitForFunction('window.Sentry');
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kind of unrelated but this kept flaking every now and then, just fixing this here...


const clickPromise = page.locator('#script-error-btn').click();

Expand Down
1 change: 1 addition & 0 deletions docs/migration/draft-v9-migration-guide.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
- Deprecated `transactionNamingScheme` option in `requestDataIntegration`.
- Deprecated `debugIntegration`. To log outgoing events, use [Hook Options](https://docs.sentry.io/platforms/javascript/configuration/options/#hooks) (`beforeSend`, `beforeSendTransaction`, ...).
- Deprecated `sessionTimingIntegration`. To capture session durations alongside events, use [Context](https://docs.sentry.io/platforms/javascript/enriching-events/context/) (`Sentry.setContext()`).
- Deprecated `addTracingHeadersToFetchRequest` method - this was only meant for internal use and is not needed anymore.

## `@sentry/nestjs`

Expand Down
33 changes: 7 additions & 26 deletions packages/browser/src/tracing/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,18 @@ import {
SentryNonRecordingSpan,
getActiveSpan,
getClient,
getCurrentScope,
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getTraceData,
hasTracingEnabled,
instrumentFetchRequest,
setHttpStatus,
spanToJSON,
spanToTraceHeader,
startInactiveSpan,
} from '@sentry/core';
import {
BAGGAGE_HEADER_NAME,
addFetchEndInstrumentationHandler,
addFetchInstrumentationHandler,
browserPerformanceTimeOrigin,
dynamicSamplingContextToSentryBaggageHeader,
generateSentryTraceHeader,
parseUrl,
stringMatchesSomePattern,
} from '@sentry/core';
Expand Down Expand Up @@ -402,12 +396,9 @@ export function xhrCallback(
xhr.__sentry_xhr_span_id__ = span.spanContext().spanId;
spans[xhr.__sentry_xhr_span_id__] = span;

const client = getClient();

if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url) && client) {
if (xhr.setRequestHeader && shouldAttachHeaders(sentryXhrData.url) && getClient()) {
addTracingHeadersToXhrRequest(
xhr,
client,
// If performance is disabled (TWP) or there's no active root span (pageload/navigation/interaction),
// we do not want to use the span as base for the trace headers,
// which means that the headers will be generated from the scope and the sampling decision is deferred
Expand All @@ -418,22 +409,12 @@ export function xhrCallback(
return span;
}

function addTracingHeadersToXhrRequest(xhr: SentryWrappedXMLHttpRequest, client: Client, span?: Span): void {
const scope = getCurrentScope();
const isolationScope = getIsolationScope();
const { traceId, spanId, sampled, dsc } = {
...isolationScope.getPropagationContext(),
...scope.getPropagationContext(),
};

const sentryTraceHeader =
span && hasTracingEnabled() ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, we have checked for span && hasTracingEnabled(), while in fetch we did not check for hasTracingEnabled(). I opted to use this logic (just check span) everywhere now...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For browser, I think that's fine!

function addTracingHeadersToXhrRequest(xhr: SentryWrappedXMLHttpRequest, span?: Span): void {
const { 'sentry-trace': sentryTrace, baggage } = getTraceData({ span });

const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(
dsc || (span ? getDynamicSamplingContextFromSpan(span) : getDynamicSamplingContextFromClient(traceId, client)),
);

setHeaderOnXhr(xhr, sentryTraceHeader, sentryBaggageHeader);
if (sentryTrace) {
setHeaderOnXhr(xhr, sentryTrace, baggage);
}
}

function setHeaderOnXhr(
Expand Down
35 changes: 11 additions & 24 deletions packages/core/src/baseclient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,23 +32,22 @@ import type {
} from '@sentry/types';

import { getEnvelopeEndpointWithUrlEncodedAuth } from './api';
import { getIsolationScope } from './currentScopes';
import { getCurrentScope, getIsolationScope, getTraceContextFromScope } from './currentScopes';
import { DEBUG_BUILD } from './debug-build';
import { createEventEnvelope, createSessionEnvelope } from './envelope';
import type { IntegrationIndex } from './integration';
import { afterSetupIntegrations } from './integration';
import { setupIntegration, setupIntegrations } from './integration';
import type { Scope } from './scope';
import { updateSession } from './session';
import { getDynamicSamplingContextFromClient } from './tracing/dynamicSamplingContext';
import { getDynamicSamplingContextFromScope } from './tracing/dynamicSamplingContext';
import { createClientReportEnvelope } from './utils-hoist/clientreport';
import { dsnToString, makeDsn } from './utils-hoist/dsn';
import { addItemToEnvelope, createAttachmentEnvelopeItem } from './utils-hoist/envelope';
import { SentryError } from './utils-hoist/error';
import { isParameterizedString, isPlainObject, isPrimitive, isThenable } from './utils-hoist/is';
import { logger } from './utils-hoist/logger';
import { checkOrSetAlreadyCaught, uuid4 } from './utils-hoist/misc';
import { dropUndefinedKeys } from './utils-hoist/object';
import { SyncPromise, rejectedSyncPromise, resolvedSyncPromise } from './utils-hoist/syncpromise';
import { parseSampleRate } from './utils/parseSampleRate';
import { prepareEvent } from './utils/prepareEvent';
Expand Down Expand Up @@ -659,7 +658,7 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
protected _prepareEvent(
event: Event,
hint: EventHint,
currentScope?: Scope,
currentScope = getCurrentScope(),
isolationScope = getIsolationScope(),
): PromiseLike<Event | null> {
const options = this.getOptions();
Expand All @@ -679,30 +678,18 @@ export abstract class BaseClient<O extends ClientOptions> implements Client<O> {
return evt;
}

const propagationContext = {
...isolationScope.getPropagationContext(),
...(currentScope ? currentScope.getPropagationContext() : undefined),
evt.contexts = {
trace: getTraceContextFromScope(currentScope),
...evt.contexts,
};

const trace = evt.contexts && evt.contexts.trace;
if (!trace && propagationContext) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These checks were not really necessary, I believe - first, propagationContext cannot be empty here. And second, we do not need to check for trace as this will be overwritten anyhow by ...evt.contexts if it already exists.

const { traceId: trace_id, spanId, parentSpanId, dsc } = propagationContext;
evt.contexts = {
trace: dropUndefinedKeys({
trace_id,
span_id: spanId,
parent_span_id: parentSpanId,
}),
...evt.contexts,
};
const dynamicSamplingContext = getDynamicSamplingContextFromScope(this, currentScope);

const dynamicSamplingContext = dsc ? dsc : getDynamicSamplingContextFromClient(trace_id, this);
evt.sdkProcessingMetadata = {
dynamicSamplingContext,
...evt.sdkProcessingMetadata,
};

evt.sdkProcessingMetadata = {
dynamicSamplingContext,
...evt.sdkProcessingMetadata,
};
}
return evt;
});
}
Expand Down
20 changes: 19 additions & 1 deletion packages/core/src/currentScopes.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import type { Scope } from '@sentry/types';
import type { Scope, TraceContext } from '@sentry/types';
import type { Client } from '@sentry/types';
import { getAsyncContextStrategy } from './asyncContext';
import { getMainCarrier } from './carrier';
import { Scope as ScopeClass } from './scope';
import { dropUndefinedKeys } from './utils-hoist/object';
import { getGlobalSingleton } from './utils-hoist/worldwide';

/**
Expand Down Expand Up @@ -120,3 +121,20 @@ export function withIsolationScope<T>(
export function getClient<C extends Client>(): C | undefined {
return getCurrentScope().getClient<C>();
}

/**
* Get a trace context for the given scope.
*/
export function getTraceContextFromScope(scope: Scope): TraceContext {
const propagationContext = scope.getPropagationContext();

const { traceId, spanId, parentSpanId } = propagationContext;

const traceContext: TraceContext = dropUndefinedKeys({
trace_id: traceId,
span_id: spanId,
parent_span_id: parentSpanId,
});

return traceContext;
}
89 changes: 38 additions & 51 deletions packages/core/src/fetch.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,14 @@
import type { Client, HandlerDataFetch, Scope, Span, SpanOrigin } from '@sentry/types';
import { getClient, getCurrentScope, getIsolationScope } from './currentScopes';
import { getClient } from './currentScopes';
import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from './semanticAttributes';
import {
SPAN_STATUS_ERROR,
getDynamicSamplingContextFromClient,
getDynamicSamplingContextFromSpan,
setHttpStatus,
startInactiveSpan,
} from './tracing';
import { SPAN_STATUS_ERROR, setHttpStatus, startInactiveSpan } from './tracing';
import { SentryNonRecordingSpan } from './tracing/sentryNonRecordingSpan';
import {
BAGGAGE_HEADER_NAME,
SENTRY_BAGGAGE_KEY_PREFIX,
dynamicSamplingContextToSentryBaggageHeader,
} from './utils-hoist/baggage';
import { BAGGAGE_HEADER_NAME, SENTRY_BAGGAGE_KEY_PREFIX } from './utils-hoist/baggage';
import { isInstanceOf } from './utils-hoist/is';
import { generateSentryTraceHeader } from './utils-hoist/tracing';
import { parseUrl } from './utils-hoist/url';
import { hasTracingEnabled } from './utils/hasTracingEnabled';
import { getActiveSpan, spanToTraceHeader } from './utils/spanUtils';
import { getActiveSpan } from './utils/spanUtils';
import { getTraceData } from './utils/traceData';

type PolymorphicRequestHeaders =
| Record<string, string | undefined>
Expand Down Expand Up @@ -63,9 +53,6 @@ export function instrumentFetchRequest(
return undefined;
}

const scope = getCurrentScope();
const client = getClient();

const { method, url } = handlerData.fetchData;

const fullUrl = getFullURL(url);
Expand All @@ -92,7 +79,7 @@ export function instrumentFetchRequest(
handlerData.fetchData.__span = span.spanContext().spanId;
spans[span.spanContext().spanId] = span;

if (shouldAttachHeaders(handlerData.fetchData.url) && client) {
if (shouldAttachHeaders(handlerData.fetchData.url) && getClient()) {
const request: string | Request = handlerData.args[0];

// In case the user hasn't set the second argument of a fetch call we default it to `{}`.
Expand All @@ -101,10 +88,10 @@ export function instrumentFetchRequest(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const options: { [key: string]: any } = handlerData.args[1];

options.headers = addTracingHeadersToFetchRequest(
options.headers = _addTracingHeadersToFetchRequest(
request,
client,
scope,
undefined,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these just remain for backwards compatibility, because addTracingHeadersToFetchRequest is exported :/

undefined,
options,
// If performance is disabled (TWP) or there's no active root span (pageload/navigation/interaction),
// we do not want to use the span as base for the trace headers,
Expand All @@ -117,12 +104,19 @@ export function instrumentFetchRequest(
}

/**
* Adds sentry-trace and baggage headers to the various forms of fetch headers
* Adds sentry-trace and baggage headers to the various forms of fetch headers.
*
* @deprecated This function will not be exported anymore in v9.
*/
export const addTracingHeadersToFetchRequest = _addTracingHeadersToFetchRequest;

/**
* Adds sentry-trace and baggage headers to the various forms of fetch headers.
*/
export function addTracingHeadersToFetchRequest(
function _addTracingHeadersToFetchRequest(
request: string | unknown, // unknown is actually type Request but we can't export DOM types from this package,
client: Client,
scope: Scope,
_client: Client | undefined,
_scope: Scope | undefined,
fetchOptionsObj: {
headers?:
| {
Expand All @@ -132,44 +126,37 @@ export function addTracingHeadersToFetchRequest(
},
span?: Span,
): PolymorphicRequestHeaders | undefined {
const isolationScope = getIsolationScope();
const traceHeaders = getTraceData({ span });
const sentryTrace = traceHeaders['sentry-trace'];
const baggage = traceHeaders.baggage;

const { traceId, spanId, sampled, dsc } = {
...isolationScope.getPropagationContext(),
...scope.getPropagationContext(),
};

const sentryTraceHeader = span ? spanToTraceHeader(span) : generateSentryTraceHeader(traceId, spanId, sampled);

const sentryBaggageHeader = dynamicSamplingContextToSentryBaggageHeader(
dsc || (span ? getDynamicSamplingContextFromSpan(span) : getDynamicSamplingContextFromClient(traceId, client)),
);
// Nothing to do, we just return the existing headers untouched
if (!sentryTrace) {
return fetchOptionsObj && (fetchOptionsObj.headers as PolymorphicRequestHeaders);
}

const headers =
fetchOptionsObj.headers ||
(typeof Request !== 'undefined' && isInstanceOf(request, Request) ? (request as Request).headers : undefined);

if (!headers) {
return { 'sentry-trace': sentryTraceHeader, baggage: sentryBaggageHeader };
return { ...traceHeaders };
} else if (typeof Headers !== 'undefined' && isInstanceOf(headers, Headers)) {
const newHeaders = new Headers(headers as Headers);
newHeaders.set('sentry-trace', sentryTrace);

newHeaders.set('sentry-trace', sentryTraceHeader);

if (sentryBaggageHeader) {
if (baggage) {
const prevBaggageHeader = newHeaders.get(BAGGAGE_HEADER_NAME);
if (prevBaggageHeader) {
const prevHeaderStrippedFromSentryBaggage = stripBaggageHeaderOfSentryBaggageValues(prevBaggageHeader);
newHeaders.set(
BAGGAGE_HEADER_NAME,
// If there are non-sentry entries (i.e. if the stripped string is non-empty/truthy) combine the stripped header and sentry baggage header
// otherwise just set the sentry baggage header
prevHeaderStrippedFromSentryBaggage
? `${prevHeaderStrippedFromSentryBaggage},${sentryBaggageHeader}`
: sentryBaggageHeader,
prevHeaderStrippedFromSentryBaggage ? `${prevHeaderStrippedFromSentryBaggage},${baggage}` : baggage,
);
} else {
newHeaders.set(BAGGAGE_HEADER_NAME, sentryBaggageHeader);
newHeaders.set(BAGGAGE_HEADER_NAME, baggage);
}
}

Expand All @@ -191,13 +178,13 @@ export function addTracingHeadersToFetchRequest(
}
}),
// Attach the new sentry-trace header
['sentry-trace', sentryTraceHeader],
['sentry-trace', sentryTrace],
];

if (sentryBaggageHeader) {
if (baggage) {
// If there are multiple entries with the same key, the browser will merge the values into a single request header.
// Its therefore safe to simply push a "baggage" entry, even though there might already be another baggage header.
newHeaders.push([BAGGAGE_HEADER_NAME, sentryBaggageHeader]);
newHeaders.push([BAGGAGE_HEADER_NAME, baggage]);
}

return newHeaders as PolymorphicRequestHeaders;
Expand All @@ -215,13 +202,13 @@ export function addTracingHeadersToFetchRequest(
newBaggageHeaders.push(stripBaggageHeaderOfSentryBaggageValues(existingBaggageHeader));
}

if (sentryBaggageHeader) {
newBaggageHeaders.push(sentryBaggageHeader);
if (baggage) {
newBaggageHeaders.push(baggage);
}

return {
...(headers as Exclude<typeof headers, Headers>),
'sentry-trace': sentryTraceHeader,
'sentry-trace': sentryTrace,
baggage: newBaggageHeaders.length > 0 ? newBaggageHeaders.join(',') : undefined,
};
}
Expand Down
7 changes: 6 additions & 1 deletion packages/core/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ export {
withScope,
withIsolationScope,
getClient,
getTraceContextFromScope,
} from './currentScopes';
export {
getDefaultCurrentScope,
Expand Down Expand Up @@ -111,7 +112,11 @@ export type { MetricData } from '@sentry/types';
export { metricsDefault } from './metrics/exports-default';
export { BrowserMetricsAggregator } from './metrics/browser-aggregator';
export { getMetricSummaryJsonForSpan } from './metrics/metric-summary';
export { addTracingHeadersToFetchRequest, instrumentFetchRequest } from './fetch';
export {
// eslint-disable-next-line deprecation/deprecation
addTracingHeadersToFetchRequest,
instrumentFetchRequest,
} from './fetch';
export { trpcMiddleware } from './trpc';
export { captureFeedback } from './feedback';

Expand Down
Loading
Loading