fix(propagator-jaeger):extract 1 digit traceFlag(0) return 1 #2906
Conversation
|
|
packages/opentelemetry-propagator-jaeger/src/JaegerPropagator.ts
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## main #2906 +/- ##
=======================================
Coverage 92.81% 92.81%
=======================================
Files 184 184
Lines 6066 6066
Branches 1296 1296
=======================================
Hits 5630 5630
Misses 436 436
|
|
@dyladan CHANGELOG done |
|
@dyladan lint fixed |
| @@ -159,7 +159,7 @@ describe('JaegerPropagator', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it('should extract context of a sampled span from carrier with 1 bit flag', () => { | |||
| it('should extract context of a sampled span from carrier with 1 bit flag(1)', () => { | |||
There was a problem hiding this comment.
nit: What does it mean "1 bit flag"? Should it say "1 hex digit flag" or "1 character flag"?
There was a problem hiding this comment.
Yes, the original test description string is a bit ambiguous.
I think it would be more accurate to use "a single digit character" to describe this, right?
There was a problem hiding this comment.
I think it is "with flag 1 set" and 1 is just the example. Could easily have been 3
There was a problem hiding this comment.
Is there anything else I can do?
There are very few flag bits that can cause problems in production
There was a problem hiding this comment.
| it('should extract context of a sampled span from carrier with 1 bit flag(1)', () => { | |
| it('should extract context of a sampled span from carrier with sampled bit (bit 1) set', () => { |
There was a problem hiding this comment.
Actually, I think the essence of the test is to check that the flags work when encoded with a single hex character (the thing this PR fixes).
The previous test ("should extract context of a sampled span from carrier") covers the case where flags are 2 digits.
I am good with any name, it's really a nit, don't want to block anything.
If you change the name, please do it also for the other (new) test as well.
There was a problem hiding this comment.
Agree
This is just a description issue, and any developer who sees this test case will not misunderstand it.
We need to solve the problem that the traceFlag length can be 1 and 2 because of the standard definition and compatibility.
In fact, there are four possible values, 0/00/01/1. If some value such as 02 or 2 appear, it is not the library's problem, but the developer's problem who generated the trace header
There was a problem hiding this comment.
Not sure I understand.
The PR is great, I only suggested improving the test name as it is currently not very clear.
If you want to do it - amazing. If not, let's merge it as is :)
There was a problem hiding this comment.
let's merge it as is :)
Which problem is this PR solving?
propagator-jaeger extract header(traceFlag 0) returned result is incorrect, but traceFlag 1 is ok
Fixes # (issue)
Short description of the changes
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Checklist:
Documentation has been updated