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

Handle distributed context headers in custom propagator #1533

Merged

Conversation

greenEkatherine
Copy link
Contributor

Resolves #1497

Added ReverseProxyPropagator that handles headers removal. Default inner implementation removes distributed context headers. It can be changed by overriding Propagator property in ForwarderHttpClientFactory which is used to set up handler.ActivityHeadersPropagator.

@greenEkatherine greenEkatherine force-pushed the distributed-tracing-propagator branch from 58a7e18 to 2f67321 Compare January 26, 2022 18:17
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

We should update the direct-forwarding docs to include changing this property on the SocketsHttpHandler as well:
https://github.com/microsoft/reverse-proxy/blob/96ed54fd0286638838edb1543333a00dbb3c595b/docs/docfx/articles/direct-forwarding.md?plain=1#L51-L57

As for the breaking-change: anyone using direct forwarding will have to set the ActivityHeadersPropagator property to match existing behavior.

@MihaZupan MihaZupan added this to the YARP 1.1.0 milestone Jan 26, 2022
Katya Sokolova and others added 2 commits January 27, 2022 10:46

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@greenEkatherine
Copy link
Contributor Author

We should update the direct-forwarding docs to include changing this property on the SocketsHttpHandler as well: https://github.com/microsoft/reverse-proxy/blob/96ed54fd0286638838edb1543333a00dbb3c595b/docs/docfx/articles/direct-forwarding.md?plain=1#L51-L57

I noticed that example there doesn't match with Direct.Sample and not only with this new property. Should I update Direct.Sample as well?

@MihaZupan
Copy link
Member

MihaZupan commented Jan 27, 2022

Yes please.

We should also consider updating https://microsoft.github.io/reverse-proxy/articles/header-guidelines.html#traceparent-request-id-tracestate-baggage-correlation-context to mention you can turn header removal off by calling

.ConfigureHttpHandler((_, handler) => handler.Propagator = null)

(might not be the exact syntax) in Startup

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
Copy link
Member

@MihaZupan MihaZupan left a comment

Choose a reason for hiding this comment

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

Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>
@greenEkatherine
Copy link
Contributor Author

@greenEkatherine greenEkatherine force-pushed the distributed-tracing-propagator branch from eae5f30 to 2b905fd Compare January 27, 2022 22:24
@greenEkatherine greenEkatherine merged commit 33eadc6 into dotnet:main Jan 28, 2022
@MihaZupan MihaZupan modified the milestones: YARP 1.1.0, YARP 1.1.0-RC1 Mar 2, 2022
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.

YARP is filtering headers (with common names) because of distributed tracing
3 participants