-
Notifications
You must be signed in to change notification settings - Fork 805
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
chore(http-propagation): reduce complexity of traceparent parsing #1837
chore(http-propagation): reduce complexity of traceparent parsing #1837
Conversation
|
Does it make sense to have tests to extract future versions if we don't know the format? I'd rewrite that test to not extract the context. |
8386bb9
to
772a7a3
Compare
772a7a3
to
e6a05de
Compare
Codecov Report
@@ Coverage Diff @@
## master #1837 +/- ##
==========================================
- Coverage 92.66% 92.28% -0.39%
==========================================
Files 174 165 -9
Lines 6040 5480 -560
Branches 1284 1172 -112
==========================================
- Hits 5597 5057 -540
+ Misses 443 423 -20 |
packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
Outdated
Show resolved
Hide resolved
83bb84b
to
5fb81c6
Compare
packages/opentelemetry-core/src/context/propagation/HttpTraceContext.ts
Outdated
Show resolved
Hide resolved
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.
what about performance, does it somehow impact that, hopefully it is not worst ?,
I did some benchmarks during the refactoring. Sadly I somehow discarded the benchmark script. Valid traceparent values were about 20% faster. Invalid ones 10 times faster. If we care about performance even more, we could remove the |
Lint failure is no fault of this PR |
Which problem is this PR solving?
While writing a custom propagator for some job queue, I went trough the code and found that the traceparent extraction looked rather complex instead of just relying on a regex to simplify the logic. By merging all the regexes into one, we harden the functionality as less can go wrong.
Short description of the changes
I've rewritten the transparent header extraction to use one regex to be less susceptible for errors and attacks. e.g. when spoofing the header with
traceparent: '-----------...'
8000 times or more (depending on header limit), it won't generate an array and allocate memory & block the event loop while parsing.