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

Tracing: add traceid_128bit support for jaeger #4942

Merged

Conversation

hanjm
Copy link
Member

@hanjm hanjm commented Dec 11, 2021

The OpenTelemetry jaegerreceiver always write 128bit traceID to backend https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/v0.41.0/pkg/translator/jaeger/jaegerthrift_to_traces.go#L95 , with leading zero.
like '00000000000000007aeab04b2badb0bd', but jaeger query will parse it to 64bit traceid to search https://github.com/jaegertracing/jaeger/blob/main/model/ids.go#L51.
One of solution is make jaeger client use 1238bit traceID.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  1. add traceid_128bit field to tracing config, align to same field in jaeger config .

Verification

@hanjm hanjm force-pushed the feature/jimmiehan-add-jaeger-config-gen128bit branch 2 times, most recently from e5fb2ae to 217bcee Compare December 11, 2021 13:38
@hanjm hanjm changed the title Tracing: add Gen128Bit support for jaeger Tracing: add traceid_128bit support for jaeger. Dec 11, 2021
@hanjm hanjm changed the title Tracing: add traceid_128bit support for jaeger. Tracing: add traceid_128bit support for jaeger Dec 11, 2021
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

Makes sense 👍
LGTM once CI is green.

Signed-off-by: Jimmie Han <hanjinming@outlook.com>
Signed-off-by: Jimmie Han <hanjinming@outlook.com>
@hanjm hanjm force-pushed the feature/jimmiehan-add-jaeger-config-gen128bit branch from 217bcee to 314a1b7 Compare December 13, 2021 13:01
@hanjm
Copy link
Member Author

hanjm commented Dec 13, 2021

Thank @yeya24 , ci is OK.

@hanjm hanjm requested a review from yeya24 December 14, 2021 00:21
@yeya24 yeya24 enabled auto-merge (squash) December 14, 2021 08:39
@yeya24 yeya24 merged commit d837809 into thanos-io:main Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants