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

chore(http-propagation): reduce complexity of traceparent parsing #1837

Merged

Conversation

marcbachmann
Copy link
Contributor

@marcbachmann marcbachmann commented Jan 16, 2021

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.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 16, 2021

CLA Signed

The committers are authorized under a signed CLA.

@vmarchaud vmarchaud changed the title chore(http-instrumentation): reduce complexity of traceparent parsing chore(http-propagation): reduce complexity of traceparent parsing Jan 16, 2021
@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jan 17, 2021

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.
edit: fix is coming. according to the spec a version should be forward-compatible: https://www.w3.org/TR/trace-context/#versioning-of-traceparent
edit2: and should be fixed. Weirdly is, I can't run the tests locally. I think it's currently broken because of the coming v1.

@marcbachmann marcbachmann force-pushed the simplify-traceparent-parsing branch from 8386bb9 to 772a7a3 Compare January 17, 2021 13:17
@marcbachmann marcbachmann force-pushed the simplify-traceparent-parsing branch from 772a7a3 to e6a05de Compare January 17, 2021 13:26
@codecov
Copy link

codecov bot commented Jan 17, 2021

Codecov Report

Merging #1837 (867b42d) into master (31ab012) will decrease coverage by 0.38%.
The diff coverage is 100.00%.

@@            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     
Impacted Files Coverage Δ
...y-core/src/context/propagation/HttpTraceContext.ts 100.00% <100.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts
...s/opentelemetry-instrumentation-fetch/src/fetch.ts
...kages/opentelemetry-web/src/StackContextManager.ts
packages/opentelemetry-web/src/utils.ts
...mentation-xml-http-request/src/enums/EventNames.ts
...ackages/opentelemetry-web/src/WebTracerProvider.ts
...ntelemetry-web/src/enums/PerformanceTimingNames.ts
...emetry-instrumentation-xml-http-request/src/xhr.ts
packages/opentelemetry-web/src/types.ts

@marcbachmann marcbachmann force-pushed the simplify-traceparent-parsing branch from 83bb84b to 5fb81c6 Compare January 19, 2021 01:17
@vmarchaud vmarchaud requested a review from johnbley as a code owner January 20, 2021 17:54
Copy link
Member

@obecny obecny left a 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 ?,

@marcbachmann
Copy link
Contributor Author

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 \s at start and end, which are not needed as the http parser removes whitespaces before header values.

@dyladan dyladan added the enhancement New feature or request label Jan 25, 2021
@dyladan
Copy link
Member

dyladan commented Jan 25, 2021

Lint failure is no fault of this PR

@dyladan dyladan merged commit bf99144 into open-telemetry:master Jan 25, 2021
@marcbachmann marcbachmann deleted the simplify-traceparent-parsing branch January 25, 2021 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants