Skip to content

Conversation

@dmehala
Copy link
Collaborator

@dmehala dmehala commented Jan 24, 2025

Description

std::regex is notoriously known to be slow. This custom parser implementation offers significantly better performance.

Benchmark

Bench: https://quick-bench.com/q/XJw86Zh_SBDpQ3xhI2nrY-k-Xbc
Result:
image

TODO

  • fuzz me!

std::regex is notoriously known to be slow. This custom parser
implementation offers significantly better performance.
@dmehala dmehala changed the title perf: parse w3c traceparent without regex perf: parse w3c traceparent using a customer parser Jan 24, 2025
@dmehala dmehala marked this pull request as ready for review January 24, 2025 16:40
@dmehala dmehala requested a review from a team as a code owner January 24, 2025 16:40
@dmehala dmehala requested review from dubloom and zacharycmontoya and removed request for a team January 24, 2025 16:40
Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@pablomartinezbernardo pablomartinezbernardo left a comment

Choose a reason for hiding this comment

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

That benchmark 😮

Apart from fuzzing, it would be cool to get some unit tests for those invalid values as well as a valid one.

@pr-commenter
Copy link

pr-commenter bot commented Jan 27, 2025

Benchmarks

Benchmark execution time: 2025-01-27 09:59:08

Comparing candidate commit 07b7518 in PR branch dmehala/faster-w3c-parsing with baseline commit 6971362 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

@pablomartinezbernardo
Copy link
Contributor

That benchmark 😮

Apart from fuzzing, it would be cool to get some unit tests for those invalid values as well as a valid one.

Oh, we did have unit tests, sorry 😓

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 92.30769% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.81%. Comparing base (6971362) to head (07b7518).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/datadog/w3c_propagation.cpp 92.30% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #178      +/-   ##
==========================================
- Coverage   93.91%   93.81%   -0.11%     
==========================================
  Files          73       73              
  Lines        4176     4202      +26     
==========================================
+ Hits         3922     3942      +20     
- Misses        254      260       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dmehala
Copy link
Collaborator Author

dmehala commented Jan 27, 2025

Benchmarks

Benchmark execution time: 2025-01-27 09:59:08

Comparing candidate commit 07b7518 in PR branch dmehala/faster-w3c-parsing with baseline commit 6971362 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

Don't be fooled by the bot, this is expected. Our macro benchmark don't measure context propagation... which is something we probably should ;)

@dmehala dmehala merged commit f590dce into main Jan 27, 2025
21 checks passed
@dmehala dmehala deleted the dmehala/faster-w3c-parsing branch January 27, 2025 10:21
@bm1549 bm1549 changed the title perf: parse w3c traceparent using a customer parser perf: parse w3c traceparent using a custom parser Apr 25, 2025
dmehala added a commit that referenced this pull request Sep 10, 2025
This addresses a regression introduced in #178, where traceparent
headers containing unsupported characters were not properly rejected and
were incorrectly treated as valid.

[APMAPI-1599]
dmehala added a commit that referenced this pull request Sep 10, 2025
This addresses a regression introduced in #178, where traceparent
headers containing unsupported characters were not properly rejected and
were incorrectly treated as valid.

[APMAPI-1599]
dmehala added a commit that referenced this pull request Sep 22, 2025
This addresses a regression introduced in #178, where traceparent
headers containing unsupported characters were not properly rejected and
were incorrectly treated as valid.

[APMAPI-1599]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants