Skip to content

Commit

Permalink
ref: Deprecate non-callback based continueTrace (#10301)
Browse files Browse the repository at this point in the history
  • Loading branch information
@lforst
lforst committed Jan 31, 2024
1 parent 9fcbb84 commit 8bec42e
Show file tree
Hide file tree
Showing 13 changed files with 392 additions and 464 deletions.
26 changes: 26 additions & 0 deletions MIGRATION.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,32 @@ be removed. Instead, use the new performance APIs:

You can [read more about the new performance APIs here](./docs/v8-new-performance-apis.md).

## Deprecate variations of `Sentry.continueTrace()`

The version of `Sentry.continueTrace()` which does not take a callback argument will be removed in favor of the version
that does. Additionally, the callback argument will not receive an argument with the next major version.

Use `Sentry.continueTrace()` as follows:

```ts
app.get('/your-route', req => {
Sentry.withIsolationScope(isolationScope => {
Sentry.continueTrace(
{
sentryTrace: req.headers.get('sentry-trace'),
baggage: req.headers.get('baggage'),
},
() => {
// All events recorded in this callback will be associated with the incoming trace. For example:
Sentry.startSpan({ name: '/my-route' }, async () => {
await doExpensiveWork();
});
},
);
});
});
```

## Deprecate `Sentry.lastEventId()` and `hub.lastEventId()`

`Sentry.lastEventId()` sometimes causes race conditions, so we are deprecating it in favour of the `beforeSend`
Expand Down
168 changes: 83 additions & 85 deletions packages/astro/src/server/middleware.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,97 +94,95 @@ async function instrumentRequest(

const { method, headers } = ctx.request;

const traceCtx = continueTrace({
sentryTrace: headers.get('sentry-trace') || undefined,
baggage: headers.get('baggage'),
});
return continueTrace(
{
sentryTrace: headers.get('sentry-trace') || undefined,
baggage: headers.get('baggage'),
},
async () => {
const allHeaders: Record<string, string> = {};

const allHeaders: Record<string, string> = {};
if (options.trackHeaders) {
headers.forEach((value, key) => {
allHeaders[key] = value;
});
}

if (options.trackHeaders) {
headers.forEach((value, key) => {
allHeaders[key] = value;
});
}
if (options.trackClientIp) {
getCurrentScope().setUser({ ip_address: ctx.clientAddress });
}

if (options.trackClientIp) {
getCurrentScope().setUser({ ip_address: ctx.clientAddress });
}
try {
const interpolatedRoute = interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params);
const source = interpolatedRoute ? 'route' : 'url';
// storing res in a variable instead of directly returning is necessary to
// invoke the catch block if next() throws
const res = await startSpan(
{
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro',
},
name: `${method} ${interpolatedRoute || ctx.url.pathname}`,
op: 'http.server',
status: 'ok',
data: {
method,
url: stripUrlQueryAndFragment(ctx.url.href),
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
...(ctx.url.search && { 'http.query': ctx.url.search }),
...(ctx.url.hash && { 'http.fragment': ctx.url.hash }),
...(options.trackHeaders && { headers: allHeaders }),
},
},
async span => {
const originalResponse = await next();

try {
const interpolatedRoute = interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params);
const source = interpolatedRoute ? 'route' : 'url';
// storing res in a variable instead of directly returning is necessary to
// invoke the catch block if next() throws
const res = await startSpan(
{
...traceCtx,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro',
},
name: `${method} ${interpolatedRoute || ctx.url.pathname}`,
op: 'http.server',
status: 'ok',
metadata: {
// eslint-disable-next-line deprecation/deprecation
...traceCtx?.metadata,
},
data: {
method,
url: stripUrlQueryAndFragment(ctx.url.href),
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source,
...(ctx.url.search && { 'http.query': ctx.url.search }),
...(ctx.url.hash && { 'http.fragment': ctx.url.hash }),
...(options.trackHeaders && { headers: allHeaders }),
},
},
async span => {
const originalResponse = await next();

if (span && originalResponse.status) {
setHttpStatus(span, originalResponse.status);
}

const scope = getCurrentScope();
const client = getClient();
const contentType = originalResponse.headers.get('content-type');

const isPageloadRequest = contentType && contentType.startsWith('text/html');
if (!isPageloadRequest || !client) {
return originalResponse;
}

// Type case necessary b/c the body's ReadableStream type doesn't include
// the async iterator that is actually available in Node
// We later on use the async iterator to read the body chunks
// see https://github.com/microsoft/TypeScript/issues/39051
const originalBody = originalResponse.body as NodeJS.ReadableStream | null;
if (!originalBody) {
return originalResponse;
}

const decoder = new TextDecoder();

const newResponseStream = new ReadableStream({
start: async controller => {
for await (const chunk of originalBody) {
const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true });
const modifiedHtml = addMetaTagToHead(html, scope, client, span);
controller.enqueue(new TextEncoder().encode(modifiedHtml));
if (span && originalResponse.status) {
setHttpStatus(span, originalResponse.status);
}
controller.close();
},
});

return new Response(newResponseStream, originalResponse);
},
);
return res;
} catch (e) {
sendErrorToSentry(e);
throw e;
}
// TODO: flush if serverless (first extract function)
const scope = getCurrentScope();
const client = getClient();
const contentType = originalResponse.headers.get('content-type');

const isPageloadRequest = contentType && contentType.startsWith('text/html');
if (!isPageloadRequest || !client) {
return originalResponse;
}

// Type case necessary b/c the body's ReadableStream type doesn't include
// the async iterator that is actually available in Node
// We later on use the async iterator to read the body chunks
// see https://github.com/microsoft/TypeScript/issues/39051
const originalBody = originalResponse.body as NodeJS.ReadableStream | null;
if (!originalBody) {
return originalResponse;
}

const decoder = new TextDecoder();

const newResponseStream = new ReadableStream({
start: async controller => {
for await (const chunk of originalBody) {
const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true });
const modifiedHtml = addMetaTagToHead(html, scope, client, span);
controller.enqueue(new TextEncoder().encode(modifiedHtml));
}
controller.close();
},
});

return new Response(newResponseStream, originalResponse);
},
);
return res;
} catch (e) {
sendErrorToSentry(e);
throw e;
}
// TODO: flush if serverless (first extract function)
},
);
}

/**
Expand Down
39 changes: 0 additions & 39 deletions packages/astro/test/server/middleware.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ describe('sentryMiddleware', () => {
url: 'https://mydomain.io/users/123/details',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
},
metadata: {},
name: 'GET /users/[id]/details',
op: 'http.server',
status: 'ok',
Expand Down Expand Up @@ -104,7 +103,6 @@ describe('sentryMiddleware', () => {
url: 'http://localhost:1234/a%xx',
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url',
},
metadata: {},
name: 'GET a%xx',
op: 'http.server',
status: 'ok',
Expand Down Expand Up @@ -144,43 +142,6 @@ describe('sentryMiddleware', () => {
});
});

it('attaches tracing headers', async () => {
const middleware = handleRequest();
const ctx = {
request: {
method: 'GET',
url: '/users',
headers: new Headers({
'sentry-trace': '12345678901234567890123456789012-1234567890123456-1',
baggage: 'sentry-release=1.0.0',
}),
},
params: {},
url: new URL('https://myDomain.io/users/'),
};
const next = vi.fn(() => nextResult);

// @ts-expect-error, a partial ctx object is fine here
await middleware(ctx, next);

expect(startSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
data: expect.objectContaining({
[SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route',
}),
metadata: {
dynamicSamplingContext: {
release: '1.0.0',
},
},
parentSampled: true,
parentSpanId: '1234567890123456',
traceId: '12345678901234567890123456789012',
}),
expect.any(Function), // the `next` function
);
});

it('attaches client IP and request headers if options are set', async () => {
const middleware = handleRequest({ trackClientIp: true, trackHeaders: true });
const ctx = {
Expand Down
88 changes: 59 additions & 29 deletions packages/core/src/tracing/trace.ts
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import type { Span, SpanTimeInput, StartSpanOptions, TransactionContext } from '@sentry/types';

import type { propagationContextFromHeaders } from '@sentry/utils';
import { dropUndefinedKeys, logger, tracingContextFromHeaders } from '@sentry/utils';

import { DEBUG_BUILD } from '../debug-build';
Expand Down Expand Up @@ -225,31 +224,58 @@ export function getActiveSpan(): Span | undefined {
return getCurrentScope().getSpan();
}

export function continueTrace({
sentryTrace,
baggage,
}: {
sentryTrace: Parameters<typeof propagationContextFromHeaders>[0];
baggage: Parameters<typeof propagationContextFromHeaders>[1];
}): Partial<TransactionContext>;
export function continueTrace<V>(
{
interface ContinueTrace {
/**
* Continue a trace from `sentry-trace` and `baggage` values.
* These values can be obtained from incoming request headers,
* or in the browser from `<meta name="sentry-trace">` and `<meta name="baggage">` HTML tags.
*
* @deprecated Use the version of this function taking a callback as second parameter instead:
*
* ```
* Sentry.continueTrace(sentryTrace: '...', baggage: '...' }, () => {
* // ...
* })
* ```
*
*/
({
sentryTrace,
baggage,
}: {
sentryTrace: Parameters<typeof propagationContextFromHeaders>[0];
baggage: Parameters<typeof propagationContextFromHeaders>[1];
},
callback: (transactionContext: Partial<TransactionContext>) => V,
): V;
/**
* Continue a trace from `sentry-trace` and `baggage` values.
* These values can be obtained from incoming request headers,
* or in the browser from `<meta name="sentry-trace">` and `<meta name="baggage">` HTML tags.
*
* The callback receives a transactionContext that may be used for `startTransaction` or `startSpan`.
*/
export function continueTrace<V>(
// eslint-disable-next-line deprecation/deprecation
sentryTrace: Parameters<typeof tracingContextFromHeaders>[0];
// eslint-disable-next-line deprecation/deprecation
baggage: Parameters<typeof tracingContextFromHeaders>[1];
}): Partial<TransactionContext>;

/**
* Continue a trace from `sentry-trace` and `baggage` values.
* These values can be obtained from incoming request headers, or in the browser from `<meta name="sentry-trace">`
* and `<meta name="baggage">` HTML tags.
*
* Spans started with `startSpan`, `startSpanManual` and `startInactiveSpan`, within the callback will automatically
* be attached to the incoming trace.
*
* Deprecation notice: In the next major version of the SDK the provided callback will not receive a transaction
* context argument.
*/
<V>(
{
sentryTrace,
baggage,
}: {
// eslint-disable-next-line deprecation/deprecation
sentryTrace: Parameters<typeof tracingContextFromHeaders>[0];
// eslint-disable-next-line deprecation/deprecation
baggage: Parameters<typeof tracingContextFromHeaders>[1];
},
// TODO(v8): Remove parameter from this callback.
callback: (transactionContext: Partial<TransactionContext>) => V,
): V;
}

export const continueTrace: ContinueTrace = <V>(
{
sentryTrace,
baggage,
Expand All @@ -260,12 +286,14 @@ export function continueTrace<V>(
baggage: Parameters<typeof tracingContextFromHeaders>[1];
},
callback?: (transactionContext: Partial<TransactionContext>) => V,
): V | Partial<TransactionContext> {
): V | Partial<TransactionContext> => {
// TODO(v8): Change this function so it doesn't do anything besides setting the propagation context on the current scope:
/*
const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
getCurrentScope().setPropagationContext(propagationContext);
return;
return withScope((scope) => {
const propagationContext = propagationContextFromHeaders(sentryTrace, baggage);
scope.setPropagationContext(propagationContext);
return callback();
})
*/

const currentScope = getCurrentScope();
Expand Down Expand Up @@ -293,8 +321,10 @@ export function continueTrace<V>(
return transactionContext;
}

return callback(transactionContext);
}
return runWithAsyncContext(() => {
return callback(transactionContext);
});
};

function createChildSpanOrTransaction(
hub: Hub,
Expand Down
Loading

0 comments on commit 8bec42e

Please sign in to comment.