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.

Sign up for GitHub

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):extract 1 digit traceFlag(0) return 1 #2906

Merged
merged 7 commits into from
Apr 24, 2022
Merged

fix(propagator-jaeger):extract 1 digit traceFlag(0) return 1 #2906

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
377786b
fix(propagator-jaeger):extract 1 digit traceFlag(0) return 1
shmilyoo Apr 19, 2022
fbdf59a
update CHANGELOG
shmilyoo Apr 20, 2022
3224267
fix quota
shmilyoo Apr 20, 2022
080b958
Merge branch 'main' into jaeger-extract-traceFlag
shmilyoo Apr 20, 2022
9003d04
Merge branch 'main' into jaeger-extract-traceFlag
shmilyoo Apr 21, 2022
edddbfc
Parse jaeger flags with radix 16
dyladan Apr 21, 2022
4a7ca04
Merge branch 'main' into jaeger-extract-traceFlag
legendecas Apr 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ All notable changes to this project will be documented in this file.

* fix: sanitize attributes inputs [#2881](https://github.com/open-telemetry/opentelemetry-js/pull/2881) @legendecas
* fix: support earlier API versions [#2892](https://github.com/open-telemetry/opentelemetry-js/pull/2892) @dyladan
* fix: support extract one digit '0' in jaeger traceFlag [#2905](https://github.com/open-telemetry/opentelemetry-js/issues/2905) @shmilyoo

### :books: (Refine Doc)

Expand All @@ -22,6 +23,8 @@ All notable changes to this project will be documented in this file.
* chore: require changelog entry to merge PR [#2847](https://github.com/open-telemetry/opentelemetry-js/pull/2847) @dyladan
* chore: remove peer API check [#2892](https://github.com/open-telemetry/opentelemetry-js/pull/2892) @dyladan
* chore: merge lerna subdirectories into a single monorepo [#2892](https://github.com/open-telemetry/opentelemetry-js/pull/2892) @dyladan
* `opentelemetry-propagator-jaeger`
* [#2906](https://github.com/open-telemetry/opentelemetry-js/pull/2906) fix: support extract one digit '0' in jaeger traceFlag ([@shmilyoo](https://github.com/shmilyoo))

## 1.1.1

Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-propagator-jaeger/src/JaegerPropagator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ function deserializeSpanContext(serializedString: string): SpanContext | null {

const traceId = _traceId.padStart(32, '0');
const spanId = _spanId.padStart(16, '0');
const traceFlags = flags.match(/^[0-9a-f]{2}$/i) ? parseInt(flags) & 1 : 1;
const traceFlags = flags.match(/^[0-9a-f]{1,2}$/i) ? parseInt(flags) & 1 : 1;
dyladan marked this conversation as resolved.
Show resolved Hide resolved

return { traceId, spanId, isRemote: true, traceFlags };
}
17 changes: 16 additions & 1 deletion packages/opentelemetry-propagator-jaeger/test/JaegerPropagator.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)', () => {
Copy link
Member

Choose a reason for hiding this comment

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

nit: What does it mean "1 bit flag"? Should it say "1 hex digit flag" or "1 character flag"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I think it is "with flag 1 set" and 1 is just the example. Could easily have been 3

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there anything else I can do?
There are very few flag bits that can cause problems in production

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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', () => {

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

let's merge it as is :)

carrier[UBER_TRACE_ID_HEADER] =
'9c41e35aeb6d1272:45fd2a9709dadcf1:a13699e3fb724f40:1';
const extractedSpanContext = trace.getSpanContext(
Expand All @@ -174,6 +174,21 @@ describe('JaegerPropagator', () => {
});
});

it('should extract context of a sampled span from carrier with 1 bit flag(0)', () => {
carrier[UBER_TRACE_ID_HEADER] =
'9c41e35aeb6d1272:45fd2a9709dadcf1:a13699e3fb724f40:0';
const extractedSpanContext = trace.getSpanContext(
jaegerPropagator.extract(ROOT_CONTEXT, carrier, defaultTextMapGetter)
);

assert.deepStrictEqual(extractedSpanContext, {
spanId: '45fd2a9709dadcf1',
traceId: '00000000000000009c41e35aeb6d1272',
isRemote: true,
traceFlags: TraceFlags.NONE,
});
});

it('should extract context of a sampled span from UTF-8 encoded carrier', () => {
carrier[UBER_TRACE_ID_HEADER] =
'ac1f3dc3c2c0b06e%3A5ac292c4a11a163e%3Ac086aaa825821068%3A1';
Expand Down