-
Notifications
You must be signed in to change notification settings - Fork 849
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
Remove streams from B3Propagator #5326
Conversation
They are expensive in this context
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #5326 +/- ##
=========================================
Coverage 91.01% 91.01%
+ Complexity 4902 4900 -2
=========================================
Files 551 551
Lines 14487 14482 -5
Branches 1369 1370 +1
=========================================
- Hits 13185 13181 -4
Misses 905 905
+ Partials 397 396 -1
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
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.
Thanks! We don't have benchmarks on this, but even if it's not less expensive, it's definitely clearer this way.
if (ctx.isPresent()) { | ||
return ctx.get(); | ||
} | ||
return multipleHeadersExtractor.extract(context, carrier, getter).orElse(context); |
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.
An example when streams result in worse readability and worse performance!
Thanks guys. In the context I was looking at this we were pulling a header from some AMQP headers. This stream call accounted for 97% of the extract call, and ~25% of the whole consumer. I actually think this library is not really targeted at the type of application that was being instrumented in this case and it may not survive there, but removing that stream seems an easy win. |
They are expensive in this context