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

fix(propagator-jaeger): zero pad extracted trace id to 32 characters #1986

Merged
merged 1 commit into from
Apr 7, 2021
Merged

fix(propagator-jaeger): zero pad extracted trace id to 32 characters #1986

merged 1 commit into from
Apr 7, 2021

Conversation

sid-maddy
Copy link
Contributor

Which problem is this PR solving?

Fixes #1983

Short description of the changes

Ensures the Jaeger propagator context extraction conforms to the Jaeger trace context specification. Specifically, when received trace id shorter that 32 characters, it will now be zero-padded to 32 characters.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 2, 2021

CLA Signed

The committers are authorized under a signed CLA.

@sid-maddy sid-maddy changed the title Bugfix/propagator jaeger/pad trace propagator-jaeger: Zero-pad trace id to 32 characters as per specification Mar 2, 2021
@codecov
Copy link

codecov bot commented Mar 2, 2021

Codecov Report

Merging #1986 (9cce712) into main (8e2e2a9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 9cce712 differs from pull request most recent head ce54776. Consider uploading reports for the commit ce54776 to get more accurate results

@@           Coverage Diff           @@
##             main    #1986   +/-   ##
=======================================
  Coverage   93.05%   93.05%           
=======================================
  Files         154      154           
  Lines        5975     5976    +1     
  Branches     1246     1246           
=======================================
+ Hits         5560     5561    +1     
  Misses        415      415           
Impacted Files Coverage Δ
...propagator-jaeger/src/JaegerHttpTracePropagator.ts 100.00% <100.00%> (ø)

@sid-maddy sid-maddy changed the title propagator-jaeger: Zero-pad trace id to 32 characters as per specification fix(propagator-jaeger): zero pad extracted trace id to 32 characters (#1983) Mar 31, 2021
@sid-maddy sid-maddy changed the title fix(propagator-jaeger): zero pad extracted trace id to 32 characters (#1983) fix(propagator-jaeger): zero pad extracted trace id to 32 characters Mar 31, 2021
@sid-maddy sid-maddy marked this pull request as ready for review March 31, 2021 09:23
@dyladan
Copy link
Member

dyladan commented Mar 31, 2021

Sorry this slipped through the cracks and it's been so long to get reviews. Implementation looks good but I think the test isn't actually testing your change.

@dyladan dyladan added the bug Something isn't working label Apr 5, 2021
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.

FYI padStart is not supported in IE 11 but luckily from what I checked the support has been already dropped My Microsoft Team in last year and on 13th of April (very soon) Windows 10 will get Edge as security update. End of IE11 - small thing to celebrate :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

propagator-jaeger: 16 character trace-id isn't zero-padded to 32 characters as per specification
4 participants