Extract Lambda Trace Context from the Event's Step Functions Context #331
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
kimi-p
changed the title
src/trace/context.ts
Outdated
| @@ -108,6 +151,14 @@ export function extractTraceContext( | |||
| } | |||
| } | |||
| return trace; | |||
| } else { | |||
| // do not send step functions context to xray | |||
| if (stepFuncContext) { | |||
There was a problem hiding this comment.
nitpik: can we combine this with the block above in order to put the stepFuncContext related logic together. that is:
if (stepFuncContext) {
try {
addStepFunctionContextToXray(stepFuncContext);
} catch (error) {
if (error instanceof Error) {
logError("couldn't add step function metadata to xray", error as Error);
}
}
if (!trace) {
trace = readTraceFromStepFunctionsContext(stepFuncContext);
if (trace !== undefined) {
return trace;
}
}
}There was a problem hiding this comment.
Great suggestion. Updated and thanks for the review!
| @@ -55,6 +56,48 @@ export interface StepFunctionContext { | |||
| "step_function.state_retry_count": number; | |||
| } | |||
|
|
|||
| export function readTraceFromStepFunctionsContext(stepFunctionContext: StepFunctionContext): TraceContext | undefined { | |||
| return { | |||
| traceID: deterministicMd5HashToBigIntString(stepFunctionContext["step_function.execution_id"]), | |||
There was a problem hiding this comment.
So, this is an issue with the existing code, (which I wrote), but I don't like how are are indexing an object instead of using property names, I feel like it's bad javascript style. I think it was done that way originally so it would be easy to add as metadata to the APM span, (we have to use the "step_function." prefix so it's grouped in the UI) . Not a blocker for merging, but maybe an opportunity for refactoring.
src/trace/context.ts
Outdated
| } | ||
|
|
||
| export function deterministicMd5HashInBinary(s: string): string { | ||
| // Md5 here is not used as an encryption method but to generate a deterministic hash as the backend does |
There was a problem hiding this comment.
Pedantic edit, since hashing != encryption
| // Md5 here is not used as an encryption method but to generate a deterministic hash as the backend does | |
| // Md5 here is used here because we don't need a cryptographically secure hashing method but to generate the same trace/span ids as the backend does |
src/trace/context.spec.ts
Outdated
| @@ -1109,3 +1146,141 @@ describe("extractTraceContext", () => { | |||
| `); | |||
| }); | |||
| }); | |||
|
|
|||
| describe("test_hexToBinary", () => { | |||
There was a problem hiding this comment.
nit: Might be worth using a table driven test here to cut down on boilerplate:
https://jestjs.io/docs/api#describeeachtablename-fn-timeout
kimi-p
deleted the
kimi.update_lambda_span_parent_context_with_step_function_context
branch
What does this PR do?
Set Lambda span's
trace_idandparent's span_idwhen Step Functions Context is detected in the event. The corresponding Lambda subtask span is set in the backend https://github.com/DataDog/logs-backend/pull/43234.The MD5 logic on the backend is here: https://github.com/DataDog/logs-backend/blob/329f0ce114138067af8cdaef82bdbe15a06ffc58/domains/serverless/apps/logs-to-traces-reducer/src/main/java/com/dd/logs/serverless/logs_to_traces/common/HashHelper.java#L12-L32
Node: md5 is used to generate a string to be converted to trace_id in a deterministic way, so md5 is not used for any encryption purposes.
Motivation
To link Step Functions log generated span (parent) with Lambda span (child).
Testing Guidelines
Tested in staging with custom made dd-lambda-js layer. The trace is here.
Additional Notes
Types of Changes
Check all that apply