Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[serverless] Support trace context propagation for ALB target groups with MultiValueHeaders #30764

Closed
wants to merge 6 commits into from

Conversation

quietsato
Copy link
Contributor

@quietsato quietsato commented Nov 5, 2024

What does this PR do?

Adds support for extracting trace context from MultiValueHeaders in ALBTargetGroupRequest

Motivation

Currently, Datadog APM context propagation doesn't work when MultiValueHeaders is enabled in ALB target groups. This PR resolves this issue by allowing trace context propagation even when using MultiValueHeaders configuration.

Describe how to test/QA your changes

  • Enable MultiValueHeaders in ALB target group
  • Create a Lambda endpoint
  • Send a request with trace context headers to the ALB endpoint
  • Verify in Datadog's trace view that the trace context is properly propagated from ALB to Lambda

Possible Drawbacks / Trade-offs

  • Only the first value from MultiValueHeaders is used, subsequent values are ignored

Additional Notes

  • This PR includes test coverage:
    • Unit tests for MultiValueHeadersCarrier
    • Tests for MultiValueHeaders extraction in ALB target groups
    • Tests for MultiValueHeaders support in tag extraction functionality
  • Maintains backward compatibility by prioritizing the existing Headers field

https://docs.aws.amazon.com/elasticloadbalancing/latest/application/lambda-functions.html#multi-value-headers-request

@quietsato quietsato requested review from a team as code owners November 5, 2024 12:44
@bits-bot
Copy link
Collaborator

bits-bot commented Nov 5, 2024

CLA assistant check
All committers have signed the CLA.

@@ -253,6 +253,16 @@ func headersCarrier(hdrs map[string]string) (tracer.TextMapReader, error) {
return tracer.TextMapCarrier(hdrs), nil
}

// multiValueHeadersCarrier returns the tracer.TextMapReader used to extract trace
// context from a MultiValueHeaders field of form map[string][]string.
func multiValueHeadersCarrier(hdrs map[string][]string) (tracer.TextMapReader, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect you can probably use https://pkg.go.dev/github.com/DataDog/[email protected]/ddtrace/tracer#HTTPHeadersCarrier which won't require you to reinvent the wheel or do extra allocations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your suggestion!

I've made the changes in the commit 270eb93 to use HTTPHeadersCarrier from dd-trace-go.
I've removed the multiValueHeadersCarrier function and its associated tests, while ensuring that the final context extraction behavior remains covered by the existing tests.

{
name: "ALBTargetGroupRequestMultiValueHeaders",
events: []interface{}{
events.ALBTargetGroupRequest{
MultiValueHeaders: toMultiValueHeaders(headersMapAll),
},
},
expCtx: ddTraceContext,
expNoErr: true,
},

}
if ua := event.Headers["User-Agent"]; ua != "" {
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do this without an extra allocation by checking event.MultiValueHeaders["Referer"] != nil && len(event.MultiValueHeaders["Referer"]) > 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion!
I've implemented this improvement in commit aaaa24f.

Copy link
Contributor

@purple4reina purple4reina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for this PR. It's a great improvement and we really appreciate your willingness to implement it. I've made just a couple of requests for changes but otherwise this looks great.

@purple4reina
Copy link
Contributor

I reopened this PR as #31542 so the tests will run and we can merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants