aj/authorizer trace context #300
Conversation
Codecov Report
@@ Coverage Diff @@
## main #300 +/- ##
==========================================
+ Coverage 80.05% 80.31% +0.26%
==========================================
Files 36 36
Lines 1715 1758 +43
Branches 387 409 +22
==========================================
+ Hits 1373 1412 +39
- Misses 290 294 +4
Partials 52 52
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -640,3 +546,22 @@ export function convertToAPMParentID(xrayParentID: string): string | undefined { | |||
| } | |||
| return hex.toString(10); | |||
| } | |||
|
|
|||
| function exportTraceData(traceData: any): TraceContext | undefined { | |||
src/trace/listener.ts
Outdated
| @@ -150,6 +157,17 @@ export class TraceListener { | |||
| return false; | |||
| } | |||
|
|
|||
| injectAuthorizerSpan(result: any): any { | |||
| const finishTime = this.inferredSpan?.isAsync() ? this.wrappedCurrentSpan?.startTime() : Date.now(); | |||
There was a problem hiding this comment.
This logic is a bit hard to follow. Maybe explain what's going on in a comment
src/trace/span-inferrer.ts
Outdated
| @@ -52,6 +55,10 @@ export class SpanInferrer { | |||
| return "sync"; | |||
| } | |||
|
|
|||
| isTracedAuthorizerInvocation(event: any): boolean { | |||
| return event.requestContext?.authorizer?._datadog; | |||
There was a problem hiding this comment.
Is there any danger from not explicitly coalescing this into a bool?
There was a problem hiding this comment.
It could evaluate true if the key _datadog is present but empty, or an empty string or something - but the next call is wrapped in a try block anyway (to handle the said case), so I think it's okay for the usage as is - but I'll add an explicit check for !== undefined, in case someone re-uses it in the future.
… to describe when we can't create an inferred span
src/trace/span-inferrer.ts
Outdated
| this.traceWrapper.startSpan("aws.apigateway.authorizer", upstreamSpanOptions), | ||
| { isAsync: false }, | ||
| ); | ||
| upstreamAuthorizerSpan.finish(event.requestContext.timeEpoch); |
There was a problem hiding this comment.
The event.requestContext.timeEpoch header exists for HTTP API (API Gateway V2).
For REST API (API Gateway V1), the corresponding key is event.requestContext.requestTimeEpoch.
I also found out that integrationLatency is only available for REST API.
Moreover, for HTTP API the key actually becomes event.requestContext.authorizer.lambda._datadog example. So this current code here fits mostly with REST API.
Also, in both cases, the epoch time here is the original user invocation timestamp. Therefore, I was using the request time epoch + integrationlatency to get an approximation in the python version. Please let me know what do you think.
In short, my suggestion for this line is
upstreamAuthorizerSpan.finish(event.requestContext.requestTimeEpoch + event.requestContext.authorizer.integrationLatency);
| "protocol": "HTTP/1.1", | ||
| "stage": "dev", | ||
| "domainPrefix": "3gsxz7lha4", | ||
| "requestTimeEpoch": 1660939855656, |
There was a problem hiding this comment.
This is an example about the other comment
…uthorizer-trace-context
…uthorizer-trace-context
…uthorizer-trace-context
Fixes DataDog/serverless-plugin-datadog#252 for NodeJS
What does this PR do?
Encodes and decodes trace context from an authorizer function
Motivation
Trace propagation and inferred span creation for token-based authorizers:
Trace propagation and inferred span creation for request-based authorizers:
Testing Guidelines
Manual and automated testing
Additional Notes
Types of Changes
Check all that apply