Hybrid Tracing (DO NOT MERGE) - #1
Conversation
swfunc
left a comment
swfunc
left a comment
There was a problem hiding this comment.
I left a few optional notes mostly asking to clarify a few things with more comments. Overall it looks great. The tests are 💯.
| function patchMethod(mod: typeof http | typeof https, method: "get" | "request", contextService: TraceContextService) { | ||
| shimmer.wrap(mod, method, (original) => { | ||
| const fn = (arg1: any, arg2: any, arg3: any) => { | ||
| const { options, callback } = normalizeArgs(arg1, arg2, arg3); |
There was a problem hiding this comment.
Could you add a comment here to clarify what's happening with the arg normalizing / why it's needed?
| } | ||
| } | ||
|
|
||
| function normalizeArgs( |
There was a problem hiding this comment.
A docstring here would be helpful
| return { options, callback }; | ||
| } | ||
|
|
||
| function requestOptionsWithTraceContext( |
There was a problem hiding this comment.
do you think it would be more clear to rename this to something like getRequestOptionsWithTraceContext? I was a little confused at first thinking this was making the request
| @@ -0,0 +1,3 @@ | |||
| export { patchHttp } from "./patch-http"; | |||
| export { extractTraceContext as readTraceContext } from "./context"; | |||
There was a problem hiding this comment.
what's the reason for renaming this?
There was a problem hiding this comment.
Good catch, I think that was an accidental automatic refactoring.
| * Reads the trace context from either an incoming lambda event, or the process environment. | ||
| * @param event An incoming lambda event. This must have incoming trace headers in order to be read. | ||
| * @param env The process environment that may contain an xray trace id environment variable. This we be used | ||
| * if the event doesn't contain trace headers. |
There was a problem hiding this comment.
could you add something here about how the _X_AMZN_TRACE_ID is set to be unique for each execution by X-Ray? I was a little confused at first thinking the env variable was set by the user for some reason
|
|
||
| import { convertToAPMParentID, TraceContext } from "./context"; | ||
|
|
||
| export class TraceContextService { |
There was a problem hiding this comment.
Could you add a short comment here about the class?
| try { | ||
| addTraceContextToXray(trace); | ||
| } catch { | ||
| // This might fail if running in an environment where xray isn't set up, (like for local development). |
There was a problem hiding this comment.
do you think it would be helpful for customers if to add a log line here?
2 participants
This contains the first approach for hybrid tracing using node.
How it works
We implement hybrid tracing by piggbacking on AWS Xray traces. In a lambda environment, AWS sets the environment variable _X_AMZN_TRACE_ID, which usually has a value formatted like this:
We convert this, (deterministically), into Datadog hybrid tracing headers, and attach to every outgoing request.
If the lambda received the Datadog hybrid tracing headers in a request, those values are used instead of the ones derived from xray. We also save those headers into a dummy xray subsegment, (so the crawler can read them).