feat: add configurable support for span links instead of parent-child #674
Conversation
|
This looks really good! I haven't tested that it works yet, but I'm happy to do so. Currently, this would only create a span link for the first item in a batch, but span links are ideal for a span with multiple 'parents'. Here's the steps I would take to allow for multiple span links:
Happy to help or hop onto a call! |
|
@nhulston I've finally got around to adding the additional changes to this PR. I'll fix the merge conflicts, but any chance you could have another look over this now that I've added more robust support for span links? All the unit tests pass. I'm just deploying stuff to my AWS account to test it works when deployed. I might not get around to doing that before I go on PTO so wanted to leave this for you to have a look at. |
| } | ||
|
|
||
| if (spanContext === null) { | ||
| if (spanContexts === null || spanContexts.length === 0) { |
There was a problem hiding this comment.
nit: we can remove the null check here
|
|
||
| if (spanContext === null) { | ||
| if (spanContexts === null || spanContexts.length === 0) { |
| const extractor = new EventBridgeSQSEventTraceExtractor(tracerWrapper, {useSpanLinks: true} as TraceConfig); | ||
|
|
||
| const traceContext = extractor.extract(payload); | ||
| expect(traceContext).not.toBeNull(); |
There was a problem hiding this comment.
can we just add a simple test to check that each item in the traceContext array matches what we expect? so change the second test record to have a different trace ID.
Then check that traceContext[0].traceId == 7379586022458917877 and traceContext[1].traceId == <new trace id>
| const traceContext = extractor.extract(payload); | ||
| expect(traceContext).not.toBeNull(); | ||
|
|
||
| expect(traceContext?.length).toBe(2); |
There was a problem hiding this comment.
same here, we should check expect(traceContext?.[0].toTraceId()).toBe("<trace id 1>"); and expect(traceContext?.[1].toTraceId()).toBe("<trace id 2>");
| const extractor = new SNSSQSEventTraceExtractor(tracerWrapper, {useSpanLinks: true} as TraceConfig); | ||
|
|
||
| const traceContext = extractor.extract(payload); | ||
| expect(traceContext.length).toBe(2); |
| tslib "^2.6.2" | ||
|
|
||
| "@aws-crypto/[email protected]", "@aws-crypto/sha256-js@^5.2.0": | ||
| "@aws-crypto/sha256-js@^5.2.0", "@aws-crypto/[email protected]": |
There was a problem hiding this comment.
make sure to revert and not commit this file, unless you are adding a dependency but it looks like no dependency was added
There was a problem hiding this comment.
Looking great! Left a couple comments. LMK when you resolve those merge conflicts, and then we can run an e2e test to make sure all the trace context is still propagated properly
https://github.com/DataDog/serverless-e2e-tests
What does this PR do?
Adds an optional configuration option for using span links instead of the automatic parenting to the upstream trace context.
Motivation
Parent-child relationships are not always optimal when building asynchronous systems, OpenTelemetry actually recommedn using span links in most cases. Adding a configurable option allows developers to pick, for each Lambda function, if parenting or linking is the right approach.
Testing Guidelines
Added tests, need advice on best ways to test this with an actual function.
Additional Notes
Types of Changes
Check all that apply