-
Notifications
You must be signed in to change notification settings - Fork 206
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
Filter out headers not required for W3C tracing in service invocation. #642
Conversation
Codecov Report
@@ Coverage Diff @@
## master #642 +/- ##
============================================
+ Coverage 77.16% 77.25% +0.08%
- Complexity 942 946 +4
============================================
Files 88 88
Lines 3009 3012 +3
Branches 334 334
============================================
+ Hits 2322 2327 +5
+ Misses 525 523 -2
Partials 162 162
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering changes themselves seem fine, I just want to know the implications of this change. Are we forbidding all future headers? Or is this just for tracing's sake? The issue seems to imply it's just to avoid large headers?
@@ -388,6 +390,28 @@ public void invokeServiceNoHotMono() { | |||
// No exception should be thrown because did not call block() on mono above. | |||
} | |||
|
|||
@Test | |||
public void invokeServiceWithContext() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test actually check that the header is filtered out? I'm not sure if the mockInterceptor
rule will only be matched if it has just those headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It only matches if the headers match too. I tested by removing the fix and the test fails.
Great point. The intent for this is to handle W3C headers and nothing else. If we want to extend the use, we can by expanding the list. |
So, adding everything to the header is a bug, causing the large header issue too. |
Makes sense! Thanks for the clarification. |
Description
Filter out headers not required for W3C tracing in service invocation.
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #638
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: