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 tracing module's Jaeger sampling flag #3251

Merged
merged 2 commits into from
Aug 7, 2023
Merged

Fix tracing module's Jaeger sampling flag #3251

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
17d8b35
Fix tracing module's inverted jaeger sampling flags
oleiade Aug 4, 2023
5ed5b89
Add tests asserting the tracing module's propagators flags are correct
oleiade Aug 4, 2023
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
Next Next commit
Fix tracing module's inverted jaeger sampling flags
  • Loading branch information
@oleiade
oleiade committed Aug 4, 2023
commit 17d8b35c7329f725fc31429bec15098a05551e50
8 changes: 4 additions & 4 deletions js/modules/k6/experimental/tracing/propagator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,11 @@ const (
// "0 value is valid and means “root span” (when not ignored)"
JaegerRootSpanID = "0"

// JaegerSampledTraceFlag is the trace-flag value for an unsampled trace.
JaegerSampledTraceFlag = "0"
// JaegerUnsampledTraceFlag is the trace-flag value for an unsampled trace.
JaegerUnsampledTraceFlag = "0"

// JaegerUnsampledTraceFlag is the trace-flag value for a sampled trace.
JaegerUnsampledTraceFlag = "1"
// JaegerSampledTraceFlag is the trace-flag value for a sampled trace.
JaegerSampledTraceFlag = "1"
Comment on lines +72 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit strange to me. It is a boolean, isn't possible to have a single value so a single source of truth?

Copy link
Member Author

@oleiade oleiade Aug 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is a small simplification. The sampled/not sampled flag is specified as part of a set of flags: a bit within a byte.

As per the specification it could have a range of values. It just so happens that we're only using the sample flag at the moment, and encode it, at the end of the chain as a string, so keeping the constant as a string is a shortcut that avoids adding conversion code.

Happy to change it to numerical values and do the encoding instead, if you prefer; but I would do it in a different PR (this is a bug fix). Just let me know 👍

)

// JaegerPropagator is a Propagator for the Jaeger trace context header
Expand Down