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

Jump to bottom

Tracing: change baggage key to be lower kebab-case for Jaeger compatability #4018

Closed
wants to merge 3 commits into from
Closed

Tracing: change baggage key to be lower kebab-case for Jaeger compatability #4018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
556729c
change ForceTracingBaggageKey to be lower kebab-cae for Jaeger compat…
conorevans Apr 4, 2021
05ba5e9
add CHANGELOG entry
conorevans Apr 4, 2021
5ced7e8
update CHANGELOG entry
conorevans Apr 4, 2021
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,10 @@ We use _breaking :warning:_ to mark changes that are not backward compatible (re

### Changed
- [#3929](https://github.com/thanos-io/thanos/pull/3929) Store: Adds the name of the instantiated memcached client to log info

- [#3948](https://github.com/thanos-io/thanos/pull/3948) Receiver: Adjust `http_request_duration_seconds` buckets for low latency requests.
- [#3856](https://github.com/thanos-io/thanos/pull/3856) Mixin: _breaking :warning:_ Introduce flexible multi-cluster/namespace mode for alerts and dashboards. Removes jobPrefix config option. Removes `namespace` by default.
- [#3937](https://github.com/thanos-io/thanos/pull/3937) Store: Reduce memory usage for range queries.
- [#4018](https://github.com/thanos-io/thanos/pull/4018) Tracing: Update "force tracing" baggage key to be lower kebab-case for Jaeger support.

### Removed

Expand Down
4 changes: 3 additions & 1 deletion pkg/tracing/tracing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ import (
)

// ForceTracingBaggageKey - force sampling header.
const ForceTracingBaggageKey = "X-Thanos-Force-Tracing"
// Lower kebab-case for Jaeger support, per documentation:
// https://www.jaegertracing.io/docs/1.22/client-libraries/#baggage
const ForceTracingBaggageKey = "x-thanos-force-tracing"
Copy link
Member

Choose a reason for hiding this comment

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

I wonder.. can we maybe support both? It will not introduce breaking change for user too. Just let's lower case the input before checking?

Copy link
Contributor Author

@conorevans conorevans Apr 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I considered that, but I wasn't sure I saw the benefit - now I think further, backwards compatibility is preferable, for sure.

I'm not 100% sure I'm envisioning it right, so I'll just check. In pkg/tracing/http.go

// If client specified ForceTracingBaggageKey header, ensure span includes it to force tracing.
span.SetBaggageItem(ForceTracingBaggageKey, r.Header.Get(ForceTracingBaggageKey))

here we'd set to span.SetBaggageItem(strings.ToLower(ForceTracingBaggageKey),...

Then set the JaegerDebugHeader in pkg/tracing/jaeger/jaeger.go to ToLower as well.

The only other place I can see that is a forceRecorder in pkg/tracing/stackdriver/tracer.go which itself is used for a basic tracer wrapping a gcloud tracer, so I don't see it necessary there?


// traceIdResponseHeader - Trace ID response header.
const traceIDResponseHeader = "X-Thanos-Trace-Id"
Expand Down