-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(aws-serverless): Extract sentry trace data from handler context
over event
#13266
Changes from all commits
3715f11
6c2953b
39506af
9d05c00
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,29 @@ | ||
import type { TextMapGetter } from '@opentelemetry/api'; | ||
import type { Context as OtelContext } from '@opentelemetry/api'; | ||
import { context as otelContext, propagation } from '@opentelemetry/api'; | ||
import type { Scope } from '@sentry/types'; | ||
import { addExceptionMechanism } from '@sentry/utils'; | ||
import { addExceptionMechanism, isString } from '@sentry/utils'; | ||
import type { Handler } from 'aws-lambda'; | ||
import type { APIGatewayProxyEventHeaders } from 'aws-lambda'; | ||
|
||
type HandlerEvent = Parameters<Handler<{ headers?: Record<string, string> }>>[0]; | ||
type HandlerContext = Parameters<Handler>[1]; | ||
|
||
type TraceData = { | ||
'sentry-trace'?: string; | ||
baggage?: string; | ||
}; | ||
|
||
// vendored from | ||
// https://github.com/open-telemetry/opentelemetry-js-contrib/blob/main/plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts#L65-L72 | ||
const headerGetter: TextMapGetter<APIGatewayProxyEventHeaders> = { | ||
keys(carrier): string[] { | ||
return Object.keys(carrier); | ||
}, | ||
get(carrier, key: string) { | ||
return carrier[key]; | ||
}, | ||
}; | ||
|
||
/** | ||
* Marks an event as unhandled by adding a span processor to the passed scope. | ||
|
@@ -12,3 +36,51 @@ export function markEventUnhandled(scope: Scope): Scope { | |
|
||
return scope; | ||
} | ||
|
||
/** | ||
* Extracts sentry trace data from the handler `context` if available and falls | ||
* back to the `event`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can also add a short note here when this would be on context and when on event? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added an explanation, but tbh this is pretty wide open. I think different AWS services make use of different event/context usage. Hope this adds a bit more info? |
||
* | ||
* When instrumenting the Lambda function with Sentry, the sentry trace data | ||
* is placed on `context.clientContext.Custom`. Users are free to modify context | ||
* tho and provide this data via `event` or `context`. | ||
*/ | ||
export function getAwsTraceData(event: HandlerEvent, context?: HandlerContext): TraceData { | ||
const headers = event.headers || {}; | ||
|
||
const traceData: TraceData = { | ||
'sentry-trace': headers['sentry-trace'], | ||
baggage: headers.baggage, | ||
}; | ||
|
||
if (context && context.clientContext && context.clientContext.Custom) { | ||
const customContext: Record<string, unknown> = context.clientContext.Custom; | ||
const sentryTrace = isString(customContext['sentry-trace']) ? customContext['sentry-trace'] : undefined; | ||
|
||
if (sentryTrace) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. m: It will probably not matter in 99% of cases, but let's just split this into two if-blocks. So basically: const sentryTrace = ...;
const baggage = ...;
if(sentryTrace) {
traceData['sentry-trace'] = sentryTrace;
}
if(baggage) {
traceData.baggage = baggage;
} I think this is slightly more robust :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking of that, but wouldn't we potentially end up with a mix of wrong pairs, e.g. both I don't know if that's a realistic usecase tho. In core, we also throw away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, then let's keep it this way, all good! 👍 |
||
traceData['sentry-trace'] = sentryTrace; | ||
traceData.baggage = isString(customContext.baggage) ? customContext.baggage : undefined; | ||
} | ||
} | ||
|
||
return traceData; | ||
} | ||
|
||
/** | ||
* A custom event context extractor for the aws integration. It takes sentry trace data | ||
* from the context rather than the event, with the event being a fallback. | ||
* | ||
* Is only used when the handler was successfully wrapped by otel and the integration option | ||
* `disableAwsContextPropagation` is `true`. | ||
*/ | ||
export function eventContextExtractor(event: HandlerEvent, context?: HandlerContext): OtelContext { | ||
// The default context extractor tries to get sampled trace headers from HTTP headers | ||
// The otel aws integration packs these onto the context, so we try to extract them from | ||
// there instead. | ||
const httpHeaders = { | ||
...(event.headers || {}), | ||
...getAwsTraceData(event, context), | ||
}; | ||
|
||
return propagation.extract(otelContext.active(), httpHeaders, headerGetter); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,102 @@ | ||
import { eventContextExtractor, getAwsTraceData } from '../src/utils'; | ||
|
||
const mockExtractContext = jest.fn(); | ||
jest.mock('@opentelemetry/api', () => { | ||
const actualApi = jest.requireActual('@opentelemetry/api'); | ||
return { | ||
...actualApi, | ||
propagation: { | ||
extract: (...args: unknown[]) => mockExtractContext(args), | ||
}, | ||
}; | ||
}); | ||
|
||
const mockContext = { | ||
clientContext: { | ||
Custom: { | ||
'sentry-trace': '12345678901234567890123456789012-1234567890123456-1', | ||
baggage: 'sentry-environment=production', | ||
}, | ||
}, | ||
}; | ||
const mockEvent = { | ||
headers: { | ||
'sentry-trace': '12345678901234567890123456789012-1234567890123456-2', | ||
baggage: 'sentry-environment=staging', | ||
}, | ||
}; | ||
|
||
describe('getTraceData', () => { | ||
test('gets sentry trace data from the context', () => { | ||
// @ts-expect-error, a partial context object is fine here | ||
const traceData = getAwsTraceData({}, mockContext); | ||
|
||
expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-1'); | ||
expect(traceData.baggage).toEqual('sentry-environment=production'); | ||
}); | ||
|
||
test('gets sentry trace data from the context even if event has data', () => { | ||
// @ts-expect-error, a partial context object is fine here | ||
const traceData = getAwsTraceData(mockEvent, mockContext); | ||
|
||
expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-1'); | ||
expect(traceData.baggage).toEqual('sentry-environment=production'); | ||
}); | ||
|
||
test('gets sentry trace data from the event if no context is passed', () => { | ||
const traceData = getAwsTraceData(mockEvent); | ||
|
||
expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-2'); | ||
expect(traceData.baggage).toEqual('sentry-environment=staging'); | ||
}); | ||
|
||
test('gets sentry trace data from the event if the context sentry trace is undefined', () => { | ||
const traceData = getAwsTraceData(mockEvent, { | ||
// @ts-expect-error, a partial context object is fine here | ||
clientContext: { Custom: { 'sentry-trace': undefined, baggage: '' } }, | ||
}); | ||
|
||
expect(traceData['sentry-trace']).toEqual('12345678901234567890123456789012-1234567890123456-2'); | ||
expect(traceData.baggage).toEqual('sentry-environment=staging'); | ||
}); | ||
}); | ||
|
||
describe('eventContextExtractor', () => { | ||
afterEach(() => { | ||
jest.clearAllMocks(); | ||
}); | ||
|
||
test('passes sentry trace data to the propagation extractor', () => { | ||
// @ts-expect-error, a partial context object is fine here | ||
eventContextExtractor(mockEvent, mockContext); | ||
|
||
// @ts-expect-error, a partial context object is fine here | ||
const expectedTraceData = getAwsTraceData(mockEvent, mockContext); | ||
|
||
expect(mockExtractContext).toHaveBeenCalledTimes(1); | ||
expect(mockExtractContext).toHaveBeenCalledWith(expect.arrayContaining([expectedTraceData])); | ||
}); | ||
|
||
test('passes along non-sentry trace headers along', () => { | ||
eventContextExtractor( | ||
{ | ||
...mockEvent, | ||
headers: { | ||
...mockEvent.headers, | ||
'X-Custom-Header': 'Foo', | ||
}, | ||
}, | ||
// @ts-expect-error, a partial context object is fine here | ||
mockContext, | ||
); | ||
|
||
const expectedHeaders = { | ||
'X-Custom-Header': 'Foo', | ||
// @ts-expect-error, a partial context object is fine here | ||
...getAwsTraceData(mockEvent, mockContext), | ||
}; | ||
|
||
expect(mockExtractContext).toHaveBeenCalledTimes(1); | ||
expect(mockExtractContext).toHaveBeenCalledWith(expect.arrayContaining([expectedHeaders])); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just out of curiosity, why is this needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package build structure isn't right without this. It pulls this in under its own node_modules folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍