Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
7 changes: 0 additions & 7 deletions packages/browser/src/tracing/browserTracingIntegration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import {
getClient,
getCurrentScope,
getDynamicSamplingContextFromSpan,
getIsolationScope,
getLocationHref,
GLOBAL_OBJ,
hasSpansEnabled,
Expand Down Expand Up @@ -554,12 +553,6 @@ export const browserTracingIntegration = ((options: Partial<BrowserTracingOption

maybeEndActiveSpan();

getIsolationScope().setPropagationContext({
traceId: generateTraceId(),
sampleRand: Math.random(),
propagationSpanId: hasSpansEnabled() ? undefined : generateSpanId(),
});

const scope = getCurrentScope();
scope.setPropagationContext({
traceId: generateTraceId(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -937,37 +937,24 @@ describe('browserTracingIntegration', () => {
setCurrentClient(client);
client.init();

const oldIsolationScopePropCtx = getIsolationScope().getPropagationContext();
const oldCurrentScopePropCtx = getCurrentScope().getPropagationContext();

startBrowserTracingNavigationSpan(client, { name: 'test navigation span' });

const newIsolationScopePropCtx = getIsolationScope().getPropagationContext();
const newCurrentScopePropCtx = getCurrentScope().getPropagationContext();

expect(oldCurrentScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
sampleRand: expect.any(Number),
});
expect(oldIsolationScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
sampleRand: expect.any(Number),
});

expect(newCurrentScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
sampleRand: expect.any(Number),
});
expect(newIsolationScopePropCtx).toEqual({
traceId: expect.stringMatching(/[a-f0-9]{32}/),
propagationSpanId: expect.stringMatching(/[a-f0-9]{16}/),
sampleRand: expect.any(Number),
});

expect(newIsolationScopePropCtx.traceId).not.toEqual(oldIsolationScopePropCtx.traceId);
expect(newCurrentScopePropCtx.traceId).not.toEqual(oldCurrentScopePropCtx.traceId);
expect(newIsolationScopePropCtx.propagationSpanId).not.toEqual(oldIsolationScopePropCtx.propagationSpanId);
});

it("saves the span's positive sampling decision and its DSC on the propagationContext when the span finishes", () => {
Expand Down
77 changes: 1 addition & 76 deletions packages/core/src/exports.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
import type { AttributeObject, RawAttribute, RawAttributes } from './attributes';
import { getClient, getCurrentScope, getIsolationScope, withIsolationScope } from './currentScopes';
import { getClient, getCurrentScope, getIsolationScope } from './currentScopes';
import { DEBUG_BUILD } from './debug-build';
import type { CaptureContext } from './scope';
import { closeSession, makeSession, updateSession } from './session';
import { startNewTrace } from './tracing/trace';
import type { CheckIn, FinishedCheckIn, MonitorConfig } from './types/checkin';
import type { Event, EventHint } from './types/event';
import type { EventProcessor } from './types/eventprocessor';
import type { Extra, Extras } from './types/extra';
Expand All @@ -13,12 +11,9 @@ import type { Session, SessionContext } from './types/session';
import type { SeverityLevel } from './types/severity';
import type { User } from './types/user';
import { debug } from './utils/debug-logger';
import { isThenable } from './utils/is';
import { uuid4 } from './utils/misc';
import type { ExclusiveEventHintOrCaptureContext } from './utils/prepareEvent';
import { parseEventHintOrCaptureContext } from './utils/prepareEvent';
import { getCombinedScopeData } from './utils/scopeData';
import { timestampInSeconds } from './utils/time';
import { GLOBAL_OBJ } from './utils/worldwide';

/**
Expand Down Expand Up @@ -182,76 +177,6 @@ export function lastEventId(): string | undefined {
return getIsolationScope().lastEventId();
}

/**
* Create a cron monitor check in and send it to Sentry.
*
* @param checkIn An object that describes a check in.
* @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
* to create a monitor automatically when sending a check in.
*/
export function captureCheckIn(checkIn: CheckIn, upsertMonitorConfig?: MonitorConfig): string {
const scope = getCurrentScope();
const client = getClient();
if (!client) {
DEBUG_BUILD && debug.warn('Cannot capture check-in. No client defined.');
} else if (!client.captureCheckIn) {
DEBUG_BUILD && debug.warn('Cannot capture check-in. Client does not support sending check-ins.');
} else {
return client.captureCheckIn(checkIn, upsertMonitorConfig, scope);
}

return uuid4();
}

/**
* Wraps a callback with a cron monitor check in. The check in will be sent to Sentry when the callback finishes.
*
* @param monitorSlug The distinct slug of the monitor.
* @param callback Callback to be monitored
* @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
* to create a monitor automatically when sending a check in.
*/
export function withMonitor<T>(
monitorSlug: CheckIn['monitorSlug'],
callback: () => T,
upsertMonitorConfig?: MonitorConfig,
): T {
function runCallback(): T {
const checkInId = captureCheckIn({ monitorSlug, status: 'in_progress' }, upsertMonitorConfig);
const now = timestampInSeconds();

function finishCheckIn(status: FinishedCheckIn['status']): void {
captureCheckIn({ monitorSlug, status, checkInId, duration: timestampInSeconds() - now });
}
// Default behavior without isolateTrace
let maybePromiseResult: T;
try {
maybePromiseResult = callback();
} catch (e) {
finishCheckIn('error');
throw e;
}

if (isThenable(maybePromiseResult)) {
return maybePromiseResult.then(
r => {
finishCheckIn('ok');
return r;
},
e => {
finishCheckIn('error');
throw e;
},
) as T;
}
finishCheckIn('ok');

return maybePromiseResult;
}

return withIsolationScope(() => (upsertMonitorConfig?.isolateTrace ? startNewTrace(runCallback) : runCallback()));
}

/**
* Call `flush()` on the current client, if there is one. See {@link Client.flush}.
*
Expand Down
98 changes: 98 additions & 0 deletions packages/core/src/monitor.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { getClient, getCurrentScope, withIsolationScope } from './currentScopes';
import { DEBUG_BUILD } from './debug-build';
import { startNewTrace } from './tracing/trace';
import type { CheckIn, FinishedCheckIn, MonitorConfig } from './types/checkin';
import { debug } from './utils/debug-logger';
import { isThenable } from './utils/is';
import { uuid4 } from './utils/misc';
import { timestampInSeconds } from './utils/time';
import { isContinuingTrace } from './utils/tracing';

/**
* Wraps a callback with a cron monitor check in. The check in will be sent to Sentry when the callback finishes.
*
* @param monitorSlug The distinct slug of the monitor.
* @param callback Callback to be monitored
* @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
* to create a monitor automatically when sending a check in.
*/
export function withMonitor<T>(
monitorSlug: CheckIn['monitorSlug'],
callback: () => T,
upsertMonitorConfig?: MonitorConfig,
): T {
function runCallback(): T {
const checkInId = captureCheckIn({ monitorSlug, status: 'in_progress' }, upsertMonitorConfig);
const now = timestampInSeconds();

function finishCheckIn(status: FinishedCheckIn['status']): void {
captureCheckIn({ monitorSlug, status, checkInId, duration: timestampInSeconds() - now });
}
// Default behavior without isolateTrace
let maybePromiseResult: T;
try {
maybePromiseResult = callback();
} catch (e) {
finishCheckIn('error');
throw e;
}

if (isThenable(maybePromiseResult)) {
return maybePromiseResult.then(
r => {
finishCheckIn('ok');
return r;
},
e => {
finishCheckIn('error');
throw e;
},
) as T;
}
finishCheckIn('ok');

return maybePromiseResult;
}

// `withIsolationScope` gives the fork its own trace, so unless `isolateTrace` is set we restore the
// parent's trace below. Copied rather than aliased: sharing the object with the parent scope would
// let in-place writes inside the callback (e.g. the HTTP server integration assigning
// `propagationSpanId`) rewrite the parent's trace.
const oldPropagationContext = { ...getCurrentScope().getPropagationContext() };

return withIsolationScope(() => {
if (upsertMonitorConfig?.isolateTrace) {
return startNewTrace(runCallback);
}

// Mirrors the reset condition in the async context strategies: only a fork that was given a fresh
// trace needs the parent's trace put back.
const scope = getCurrentScope();
if (!isContinuingTrace(scope.getPropagationContext())) {
scope.setPropagationContext(oldPropagationContext);
}
Comment thread
cursor[bot] marked this conversation as resolved.

return runCallback();
});
}

/**
* Create a cron monitor check in and send it to Sentry.
*
* @param checkIn An object that describes a check in.
* @param upsertMonitorConfig An optional object that describes a monitor config. Use this if you want
* to create a monitor automatically when sending a check in.
*/
export function captureCheckIn(checkIn: CheckIn, upsertMonitorConfig?: MonitorConfig): string {
const scope = getCurrentScope();
const client = getClient();
if (!client) {
DEBUG_BUILD && debug.warn('Cannot capture check-in. No client defined.');
} else if (!client.captureCheckIn) {
DEBUG_BUILD && debug.warn('Cannot capture check-in. Client does not support sending check-ins.');
} else {
return client.captureCheckIn(checkIn, upsertMonitorConfig, scope);
}

return uuid4();
}
4 changes: 2 additions & 2 deletions packages/core/src/shared-exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ export * from './tracing';
export * from './semanticAttributes';
export { createEventEnvelope, createSessionEnvelope } from './envelope';
export {
captureCheckIn,
withMonitor,
captureException,
captureEvent,
captureMessage,
Expand All @@ -36,6 +34,7 @@ export {
captureSession,
addEventProcessor,
} from './exports';
export { withMonitor, captureCheckIn } from './monitor';
export {
getCurrentScope,
getIsolationScope,
Expand Down Expand Up @@ -277,6 +276,7 @@ export {
TRACEPARENT_REGEXP,
extractTraceparentData,
generateSentryTraceHeader,
isContinuingTrace,
propagationContextFromHeaders,
shouldContinueTrace,
generateTraceparentHeader,
Expand Down
12 changes: 2 additions & 10 deletions packages/core/src/tracing/trace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -367,7 +367,7 @@ function createChildOrRootSpan({
const isolationScope = getIsolationScope();

if (!hasSpansEnabled()) {
const scopePropagationContext = { ...isolationScope.getPropagationContext(), ...scope.getPropagationContext() };
const scopePropagationContext = scope.getPropagationContext();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

it seems like this removes a footgun where we potentially incorrectly merged a propagation context before here. I like it!

const traceId = parentSpan ? parentSpan.spanContext().traceId : scopePropagationContext.traceId;

// The placeholder is a thin marker; it carries no sampling decision or DSC. Both are read from
Expand Down Expand Up @@ -432,15 +432,7 @@ function createChildOrRootSpan({

freezeDscOnSpan(span, dsc);
} else {
const {
traceId,
dsc,
parentSpanId,
sampled: parentSampled,
} = {
...isolationScope.getPropagationContext(),
...scope.getPropagationContext(),
};
const { traceId, dsc, parentSpanId, sampled: parentSampled } = scope.getPropagationContext();

span = _startRootSpan(
{
Expand Down
11 changes: 11 additions & 0 deletions packages/core/src/utils/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,17 @@ export function extractTraceparentData(traceparent?: string): TraceparentData |
};
}

/**
* Whether a propagation context continues an incoming trace, rather than being the head of a new one.
*
* Both fields have to be checked. `parentSpanId` is unset when the incoming `sentry-trace` header
* carried no span id, which the header format allows. `dsc` is unset when a remote parent arrived
* without any incoming baggage. Either one on its own therefore misses a continued trace.
*/
export function isContinuingTrace(propagationContext: PropagationContext): boolean {
return !!propagationContext.parentSpanId || !!propagationContext.dsc;
}

/**
* Create a propagation context from incoming headers or
* creates a minimal new one if the headers are undefined.
Expand Down
60 changes: 60 additions & 0 deletions packages/core/test/lib/monitor.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
import { beforeEach, describe, expect, it } from 'vitest';
import { getCurrentScope, getGlobalScope, getIsolationScope } from '../../src/currentScopes';
import { withMonitor } from '../../src/monitor';
import { setCurrentClient } from '../../src/sdk';
import { getDefaultTestClientOptions, TestClient } from '../mocks/client';

describe('withMonitor', () => {
beforeEach(() => {
getCurrentScope().clear();
getIsolationScope().clear();
getGlobalScope().clear();

const client = new TestClient(getDefaultTestClientOptions({ dsn: 'https://username@domain/123' }));
setCurrentClient(client);
client.init();
});

it('keeps the parent trace when not isolating the trace', () => {
const parentTraceId = getCurrentScope().getPropagationContext().traceId;

withMonitor('cron-job', () => {
expect(getCurrentScope().getPropagationContext().traceId).toBe(parentTraceId);
});
});

it('starts a separate trace when isolateTrace is set', () => {
const parentTraceId = getCurrentScope().getPropagationContext().traceId;

withMonitor(
'cron-job',
() => {
expect(getCurrentScope().getPropagationContext().traceId).not.toBe(parentTraceId);
},
{ schedule: { type: 'crontab', value: '* * * * *' }, isolateTrace: true },
);
});

// The parent's propagation context is restored onto the monitor scope, so it
// must be copied rather than aliased. Several call sites mutate the
// propagation context in place (e.g. the Node HTTP server integration
// assigns `propagationSpanId`), which would otherwise rewrite the parent's
// trace from inside the callback.
it('does not let the callback mutate the parent propagation context', () => {
const parentScope = getCurrentScope();
const parentPropagationContext = parentScope.getPropagationContext();

withMonitor('cron-job', () => {
const propagationContext = getCurrentScope().getPropagationContext();

expect(propagationContext).not.toBe(parentPropagationContext);
expect(propagationContext).toEqual(parentPropagationContext);

propagationContext.propagationSpanId = 'deadbeefdeadbeef';
propagationContext.traceId = 'aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa';
});

expect(parentScope.getPropagationContext()).toEqual(parentPropagationContext);
expect(parentPropagationContext.propagationSpanId).toBeUndefined();
});
});
2 changes: 1 addition & 1 deletion packages/core/test/lib/sdk.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { beforeEach, describe, expect, it, type Mock, test, vi } from 'vitest';
import type { Client } from '../../src/client';
import { getCurrentScope } from '../../src/currentScopes';
import { captureCheckIn } from '../../src/exports';
import { captureCheckIn } from '../../src/monitor';
import { installedIntegrations } from '../../src/integration';
import { initAndBind, setCurrentClient } from '../../src/sdk';
import type { Integration } from '../../src/types/integration';
Expand Down
Loading
Loading