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

Fix regression introduced in #1126 (brave headers propagation) #1206

Merged

Conversation

bsideup
Copy link
Contributor

@bsideup bsideup commented Feb 5, 2019

Before #1126, the headers were eagerly set in
TraceExchangeFilterFunction#filter. After it, the side effect was
moved to lazy MonoWebClientTrace#subscribe.

However, we have everything to instrument the request in filter,
and it can be done eagerly.

This should fix #1199

…tion)

Before spring-cloud#1126, the headers were eagerly set in
`TraceExchangeFilterFunction#filter`. After it, the side effect was
moved to lazy `MonoWebClientTrace#subscribe`.

However, we have everything to instrument the request in `filter`,
and it can be done eagerly.
Copy link
Member

@spencergibb spencergibb left a comment

Choose a reason for hiding this comment

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

Test?

@bsideup
Copy link
Contributor Author

bsideup commented Feb 5, 2019

@spencergibb

Test?

That's exactly what we're discussing with @marcingrzejszczak right now in Slack :D
I do agree that this change should not be accepted without a test, but I need to sync whether I have additional time to do it (since the issue was purely in Spring Cloud Sleuth).

Will check shortly, I like writing tests 👍

@bsideup
Copy link
Contributor Author

bsideup commented Feb 5, 2019

FYI the manual testing was done with the repro project from #1199

@bsideup
Copy link
Contributor Author

bsideup commented Feb 5, 2019

@spencergibb ok, writing a test took less than 15 minutes 🎉

@codecov-io
Copy link

codecov-io commented Feb 5, 2019

Codecov Report

Merging #1206 into master will decrease coverage by <.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1206      +/-   ##
============================================
- Coverage     69.95%   69.94%   -0.01%     
  Complexity      753      753              
============================================
  Files           141      141              
  Lines          3521     3520       -1     
  Branches        383      383              
============================================
- Hits           2463     2462       -1     
  Misses          845      845              
  Partials        213      213
Impacted Files Coverage Δ Complexity Δ
...nt/web/client/TraceWebClientBeanPostProcessor.java 80.14% <81.81%> (-0.15%) 9 <0> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fb9177...78c5236. Read the comment docs.

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

Successfully merging this pull request may close these issues.

Regression: sleuth.propagation-keys aren't available in filter with WebFlux
5 participants