add multivalue headers#216
Conversation
|
The code in this PR sees all reasonable. But I believe your plan is to split them into multiple PRs? The struggle from code reviewer's perspective is that it's hard to see how all these changes are effective in the end when combined. |
|
@joeyzhao2018 For sure! Some AWS services use multivalue headers instead of regular headers. Currently, when extracting context from headers using the default extractor, we only extract from the regular headers and dont check multivalue headers. This change essentially fixes that. |
joeyzhao2018
left a comment
There was a problem hiding this comment.
LGTM. Can you por this change to https://github.com/DataDog/dd-trace-go/tree/main/contrib/aws/datadog-lambda-go too? Thank you. 🙇♂️
What does this PR do?
TLDR: This enables pulling trace context headers from multivalue headers (rather than just regular "headers"). Single value headers still take precedence
Some services pass headers through multivalue headers. Here's a sample request from an AWS Application Load Balancer (ALB):

Prior to this change, when extracting headers using the default extractor, we ignore multiValueheaders resulting in empty or sparse headers:

With the changes made, we now do not lose any headers:

and can create a valid trace context:

note that in the case that trace context values are passed via go context instead of event headers, we are still able to create a traceContext without use of multivalue headers
also, if both headers and multivalueHeaders are provided, regular headers will take precedence in the case of key conflicts
Motivation
Issue #189
APMSVLS-1
Testing Guidelines
I created some unit tests and updated existing ones. Also ran lamba functions to see if expected header was present
Additional Notes
Types of changes
Checklist