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.

Sign up for GitHub

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(instrumentation-aws-lambda): Changed capturing of X-Ray context as span link #1411

Closed
wants to merge 3 commits into from
Closed

feat(instrumentation-aws-lambda): Changed capturing of X-Ray context as span link #1411

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02b8c66
feat: changed capturing X-Ray context as span link
martinkuba Feb 23, 2023
7907788
Merge branch 'main' into lambda-change-xray-propagation
martinkuba Jul 18, 2023
d77a73d
removed disableAwsContextPropagation config
martinkuba Jul 18, 2023
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
3 changes: 1 addition & 2 deletions plugins/node/opentelemetry-instrumentation-aws-lambda/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,7 @@ In your Lambda function configuration, add or update the `NODE_OPTIONS` environm
| --- | --- | --- |
| `requestHook` | `RequestHook` (function) | Hook for adding custom attributes before lambda starts handling the request. Receives params: `span, { event, context }` |
| `responseHook` | `ResponseHook` (function) | Hook for adding custom attributes before lambda returns the response. Receives params: `span, { err?, res? }` |
| `disableAwsContextPropagation` | `boolean` | By default, this instrumentation will try to read the context from the `_X_AMZN_TRACE_ID` environment variable set by Lambda, set this to `true` to disable this behavior |
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't removing this a breaking change?

| `eventContextExtractor` | `EventContextExtractor` (function) | Function for providing custom context extractor in order to support different event types that are handled by AWS Lambda (e.g., SQS, CloudWatch, Kinesis, API Gateway). Applied only when `disableAwsContextPropagation` is set to `true`. Receives params: `event, context` |
| `eventContextExtractor` | `EventContextExtractor` (function) | Function for providing custom context extractor in order to support different event types that are handled by AWS Lambda (e.g., SQS, CloudWatch, Kinesis, API Gateway). Receives params: `event, context` |

### Hooks Usage Example

Expand Down
62 changes: 32 additions & 30 deletions plugins/node/opentelemetry-instrumentation-aws-lambda/src/instrumentation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,11 @@ import {
SpanKind,
SpanStatusCode,
TextMapGetter,
TraceFlags,
TracerProvider,
ROOT_CONTEXT,
Link,
isSpanContextValid,
TraceFlags,
} from '@opentelemetry/api';
import {
AWSXRAY_TRACE_ID_HEADER,
Expand Down Expand Up @@ -156,11 +158,12 @@ export class AwsLambdaInstrumentation extends InstrumentationBase {
const parent = AwsLambdaInstrumentation._determineParent(
event,
context,
config.disableAwsContextPropagation === true,
config.eventContextExtractor ||
AwsLambdaInstrumentation._defaultEventContextExtractor
);

const links = AwsLambdaInstrumentation._determineLinks();

const name = context.functionName;
const span = plugin.tracer.startSpan(
name,
Expand All @@ -174,6 +177,7 @@ export class AwsLambdaInstrumentation extends InstrumentationBase {
context.invokedFunctionArn
),
},
links: links,
},
parent
);
Expand Down Expand Up @@ -356,32 +360,8 @@ export class AwsLambdaInstrumentation extends InstrumentationBase {
private static _determineParent(
event: any,
context: Context,
disableAwsContextPropagation: boolean,
eventContextExtractor: EventContextExtractor
): OtelContext {
let parent: OtelContext | undefined = undefined;
if (!disableAwsContextPropagation) {
const lambdaTraceHeader = process.env[traceContextEnvironmentKey];
if (lambdaTraceHeader) {
parent = awsPropagator.extract(
otelContext.active(),
{ [AWSXRAY_TRACE_ID_HEADER]: lambdaTraceHeader },
headerGetter
);
}
if (parent) {
const spanContext = trace.getSpan(parent)?.spanContext();
if (
spanContext &&
(spanContext.traceFlags & TraceFlags.SAMPLED) === TraceFlags.SAMPLED
) {
// Trace header provided by Lambda only sampled if a sampled context was propagated from
// an upstream cloud service such as S3, or the user is using X-Ray. In these cases, we
// need to use it as the parent.
return parent;
}
}
}
const extractedContext = safeExecuteInTheMiddle(
() => eventContextExtractor(event, context),
e => {
Expand All @@ -396,10 +376,32 @@ export class AwsLambdaInstrumentation extends InstrumentationBase {
if (trace.getSpan(extractedContext)?.spanContext()) {
return extractedContext;
}
if (!parent) {
// No context in Lambda environment or HTTP headers.
return ROOT_CONTEXT;
// No context in Lambda environment or HTTP headers.
return ROOT_CONTEXT;
}

private static _determineLinks(): Link[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned that changing this behavior also being a breaking change. Will X-Ray customers who once saw their Lambda span as a direct child of the incoming context now only see it linked to the incoming context? Or will it still have a parent-child relationship (as before) but now ALSO have a link? I suppose this should have been discussed in the spec change process, but I wasn't aware of it :(

Copy link
Member

Choose a reason for hiding this comment

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

@willarmiros to be clear since you are the component owner, are you asking that we block this change? Since this comes from the spec, I would lean on the side of accepting it. Also, since this component is 0.x version it should be expected that breaking changes are possible.

let parent: OtelContext | undefined = undefined;
const lambdaTraceHeader = process.env[traceContextEnvironmentKey];
if (lambdaTraceHeader) {
parent = awsPropagator.extract(
otelContext.active(),
{ [AWSXRAY_TRACE_ID_HEADER]: lambdaTraceHeader },
headerGetter
);
if (parent) {
const spanContext = trace.getSpan(parent)?.spanContext();
if (
spanContext &&
isSpanContextValid(spanContext) &&
spanContext.traceFlags & TraceFlags.SAMPLED
) {
return [
{ context: spanContext, attributes: { source: 'x-ray-env' } },
];
}
}
}
return parent;
return [];
}
}
1 change: 0 additions & 1 deletion plugins/node/opentelemetry-instrumentation-aws-lambda/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,5 @@ export type EventContextExtractor = (
export interface AwsLambdaInstrumentationConfig extends InstrumentationConfig {
requestHook?: RequestHook;
responseHook?: ResponseHook;
disableAwsContextPropagation?: boolean;
eventContextExtractor?: EventContextExtractor;
}
110 changes: 19 additions & 91 deletions ...ns/node/opentelemetry-instrumentation-aws-lambda/test/integrations/lambda-handler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,7 @@ describe('lambda handler', () => {
});

describe('with remote parent', () => {
it('uses lambda context if sampled and no http context', async () => {
it('creates a link to a sampled lambda context', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
initializeHandler('lambda-test/async.handler');

Expand All @@ -451,14 +451,19 @@ describe('lambda handler', () => {
const [span] = spans;
assert.strictEqual(spans.length, 1);
assertSpanSuccess(span);
assert.strictEqual(
assert.notEqual(
span.spanContext().traceId,
sampledAwsSpanContext.traceId
);
assert.strictEqual(span.parentSpanId, sampledAwsSpanContext.spanId);
assert.notEqual(span.parentSpanId, sampledAwsSpanContext.spanId);

assert.strictEqual(span.links.length, 1);
const link = span.links[0];
assert.strictEqual(link.context.traceId, sampledAwsSpanContext.traceId);
assert.strictEqual(link.attributes!['source'], 'x-ray-env');
});

it('uses lambda context if unsampled and no http context', async () => {
it('does not create a link to an unsampled lambda context', async () => {
process.env[traceContextEnvironmentKey] = unsampledAwsHeader;
initializeHandler('lambda-test/async.handler');

Expand All @@ -468,38 +473,19 @@ describe('lambda handler', () => {
);
assert.strictEqual(result, 'ok');
const spans = memoryExporter.getFinishedSpans();
// Parent unsampled so no exported spans.
assert.strictEqual(spans.length, 0);
});

it('uses lambda context if sampled and http context present', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
initializeHandler('lambda-test/async.handler');

const proxyEvent = {
headers: {
traceparent: sampledHttpHeader,
},
};

const result = await lambdaRequire('lambda-test/async').handler(
proxyEvent,
ctx
);
assert.strictEqual(result, 'ok');
const spans = memoryExporter.getFinishedSpans();
const [span] = spans;
assert.strictEqual(spans.length, 1);
assertSpanSuccess(span);
assert.strictEqual(
assert.notEqual(
span.spanContext().traceId,
sampledAwsSpanContext.traceId
unsampledAwsSpanContext.traceId
);
assert.strictEqual(span.parentSpanId, sampledAwsSpanContext.spanId);
assert.notEqual(span.parentSpanId, unsampledAwsSpanContext.spanId);

assert.strictEqual(span.links.length, 0);
});

it('uses http context if sampled and lambda context unsampled', async () => {
process.env[traceContextEnvironmentKey] = unsampledAwsHeader;
it('uses http context if sampled', async () => {
initializeHandler('lambda-test/async.handler');

const proxyEvent = {
Expand All @@ -524,8 +510,7 @@ describe('lambda handler', () => {
assert.strictEqual(span.parentSpanId, sampledHttpSpanContext.spanId);
});

it('uses http context if unsampled and lambda context unsampled', async () => {
process.env[traceContextEnvironmentKey] = unsampledAwsHeader;
it('uses http context if unsampled', async () => {
initializeHandler('lambda-test/async.handler');

const proxyEvent = {
Expand All @@ -544,65 +529,12 @@ describe('lambda handler', () => {
assert.strictEqual(spans.length, 0);
});

it('ignores sampled lambda context if "disableAwsContextPropagation" config option is true', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
initializeHandler('lambda-test/async.handler', {
disableAwsContextPropagation: true,
});

const result = await lambdaRequire('lambda-test/async').handler(
'arg',
ctx
);
assert.strictEqual(result, 'ok');
const spans = memoryExporter.getFinishedSpans();
const [span] = spans;
assert.strictEqual(spans.length, 1);
assertSpanSuccess(span);
assert.notDeepStrictEqual(
span.spanContext().traceId,
sampledAwsSpanContext.traceId
);
assert.strictEqual(span.parentSpanId, undefined);
});

it('takes sampled http context over sampled lambda context if "disableAwsContextPropagation" config option is true', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
initializeHandler('lambda-test/async.handler', {
disableAwsContextPropagation: true,
});

const proxyEvent = {
headers: {
traceparent: sampledHttpHeader,
},
};

const result = await lambdaRequire('lambda-test/async').handler(
proxyEvent,
ctx
);

assert.strictEqual(result, 'ok');
const spans = memoryExporter.getFinishedSpans();
const [span] = spans;
assert.strictEqual(spans.length, 1);
assertSpanSuccess(span);
assert.strictEqual(
span.spanContext().traceId,
sampledHttpSpanContext.traceId
);
assert.strictEqual(span.parentSpanId, sampledHttpSpanContext.spanId);
});

it('takes sampled custom context over sampled lambda context if "eventContextExtractor" is defined', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
it('takes sampled custom context if "eventContextExtractor" is defined', async () => {
const customExtractor = (event: any): OtelContext => {
return propagation.extract(context.active(), event.contextCarrier);
};

initializeHandler('lambda-test/async.handler', {
disableAwsContextPropagation: true,
eventContextExtractor: customExtractor,
});

Expand All @@ -629,8 +561,7 @@ describe('lambda handler', () => {
assert.strictEqual(span.parentSpanId, sampledGenericSpanContext.spanId);
});

it('prefers to extract baggage over sampled lambda context if "eventContextExtractor" is defined', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
it('extracts baggage if "eventContextExtractor" is defined', async () => {
const customExtractor = (event: any): OtelContext => {
return propagation.extract(
context.active(),
Expand All @@ -639,7 +570,6 @@ describe('lambda handler', () => {
};

initializeHandler('lambda-test/async.handler_return_baggage', {
disableAwsContextPropagation: true,
eventContextExtractor: customExtractor,
});

Expand All @@ -660,7 +590,7 @@ describe('lambda handler', () => {
assert.strictEqual(actual, baggage);
});

it('creates trace from ROOT_CONTEXT when "disableAwsContextPropagation" is true, eventContextExtractor is provided, and no custom context is found', async () => {
it('creates trace from ROOT_CONTEXT when eventContextExtractor is provided, and no custom context is found', async () => {
process.env[traceContextEnvironmentKey] = sampledAwsHeader;
const customExtractor = (event: any): OtelContext => {
if (!event.contextCarrier) {
Expand All @@ -671,7 +601,6 @@ describe('lambda handler', () => {
};

const provider = initializeHandler('lambda-test/async.handler', {
disableAwsContextPropagation: true,
eventContextExtractor: customExtractor,
});

Expand Down Expand Up @@ -702,7 +631,6 @@ describe('lambda handler', () => {
};

initializeHandler('lambda-test/async.handler', {
disableAwsContextPropagation: true,
eventContextExtractor: customExtractor,
});

Expand Down