Use OpenTelemetry SDK in HotROD 🚗 #4187
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* replacing the jaeger-client-go ot tracer implementation with the ot-otel bridge implementation Signed-off-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us>
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: rbroggi <ro_broggi@hotmail.com>
* no need to globally register the otel tracer Signed-off-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: rbroggi <ro_broggi@hotmail.com>
Signed-off-by: Yuri Shkuro <github@ysh.us>
Codecov ReportBase: 97.11% // Head: 97.09% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4187 +/- ##
==========================================
- Coverage 97.11% 97.09% -0.02%
==========================================
Files 297 301 +4
Lines 17535 17674 +139
==========================================
+ Hits 17029 17161 +132
- Misses 407 413 +6
- Partials 99 100 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Signed-off-by: Yuri Shkuro <github@ysh.us>
yurishkuro
commented
Jan 29, 2023
| // to return an OpenTracing-compatible tracer. | ||
| func Init(serviceName string, exporterType string, logger log.Factory) opentracing.Tracer { | ||
| once.Do(func() { | ||
| otel.SetTextMapPropagator(propagation.TraceContext{}) |
There was a problem hiding this comment.
@rbroggi your PR was mostly spot-on, this was the only thing missing, so traces did not continue across services.
There was a problem hiding this comment.
Thx for reaching out. I have been quite in the shadows lately. Hope to come back to some contributions soon.
albertteoh
previously approved these changes
Jan 29, 2023
There was a problem hiding this comment.
LGTM; do you think this should be the approach (i.e. using the OTEL -> OT shim) to take for #3381?
|
@albertteoh my intention was to make this change work with the original HotROD blog post (probably time to update it to current state). But it has a section that demonstrates generation of RPC metrics by using span observer in the tracer. This functionality is now broken, but I suspect it's possible to reproduce it via an additional span processor passed to the OTEL SDK |
I don't have a strong opinion there, but it's probably better to just do a full upgrade to OTEL. I kinda wanted to try out the OT-OTEL shim in the Hotrod first, since I haven't seen any posts describing a successful use of the shim on existing code. Now that I know it works (maybe write a blog post about it), it may be worth doing a follow-up PR and move to OTEL directly. It will involve using different instrumentation libraries though. |
yurishkuro
changed the title
Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Yuri Shkuro <github@ysh.us>
5 tasks
1 task
yurishkuro
added a commit
that referenced
this pull request
Feb 9, 2023
## Which problem is this PR solving? - The baggage propagation was broken in HotROD after #4187 - This restores the baggage support by providing a few integration points with OTEL - [ ] This change will not work until OTEL Baggage & Bridge bugs are fixed: open-telemetry/opentelemetry-go#3685 ## Short description of the changes - [`index.html`] The webapp used to send baggage via `jaeger-baggage` header, this is now changed to W3C `baggage` header - [`mux.go`] The Jaeger SDK was able to accept `jaeger-baggage` header even for requests without am active trace. OTEL Bridge does not support that, so we use OTEL's Baggage propagator to manually extract the baggage into Context, and once the Bridge creates a Span, we copy OTEL baggage from Context into the Span. - All other changes are cosmetic / logging related --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
shubbham1215
pushed a commit
to shubbham1215/jaeger
that referenced
this pull request
Feb 22, 2023
Based on earlier PR jaegertracing#3390 by @rbroggi. ## Which problem is this PR solving? - Resolves jaegertracing#3380 ## Short description of the changes - Switch from jaeger-client-go SDK to OTel SDK paired with ot-otel bridge - Add cli flag to select which Otel Exporter to use (Jaeger, OTLP or stdout) - Add favicon 🚗 - Added OTEL version of rpcmetrics from jaeger-client-go - Remove dependency on github.com/uber/jaeger-lib in HotROD (addresses one of outstanding tasks in jaegertracing#3766) --------- Signed-off-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: rbroggi <ro_broggi@hotmail.com>
shubbham1215
pushed a commit
to shubbham1215/jaeger
that referenced
this pull request
Feb 22, 2023
## Which problem is this PR solving? - The baggage propagation was broken in HotROD after jaegertracing#4187 - This restores the baggage support by providing a few integration points with OTEL - [ ] This change will not work until OTEL Baggage & Bridge bugs are fixed: open-telemetry/opentelemetry-go#3685 ## Short description of the changes - [`index.html`] The webapp used to send baggage via `jaeger-baggage` header, this is now changed to W3C `baggage` header - [`mux.go`] The Jaeger SDK was able to accept `jaeger-baggage` header even for requests without am active trace. OTEL Bridge does not support that, so we use OTEL's Baggage propagator to manually extract the baggage into Context, and once the Bridge creates a Span, we copy OTEL baggage from Context into the Span. - All other changes are cosmetic / logging related --------- Signed-off-by: Yuri Shkuro <github@ysh.us>
shubbham1215
pushed a commit
to shubbham1215/jaeger
that referenced
this pull request
Mar 5, 2023
Based on earlier PR jaegertracing#3390 by @rbroggi. ## Which problem is this PR solving? - Resolves jaegertracing#3380 ## Short description of the changes - Switch from jaeger-client-go SDK to OTel SDK paired with ot-otel bridge - Add cli flag to select which Otel Exporter to use (Jaeger, OTLP or stdout) - Add favicon 🚗 - Added OTEL version of rpcmetrics from jaeger-client-go - Remove dependency on github.com/uber/jaeger-lib in HotROD (addresses one of outstanding tasks in jaegertracing#3766) --------- Signed-off-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: Yuri Shkuro <github@ysh.us> Co-authored-by: rbroggi <ro_broggi@hotmail.com> Signed-off-by: shubbham1215 <sawaikershubham@gmail.com>
shubbham1215
pushed a commit
to shubbham1215/jaeger
that referenced
this pull request
Mar 5, 2023
## Which problem is this PR solving? - The baggage propagation was broken in HotROD after jaegertracing#4187 - This restores the baggage support by providing a few integration points with OTEL - [ ] This change will not work until OTEL Baggage & Bridge bugs are fixed: open-telemetry/opentelemetry-go#3685 ## Short description of the changes - [`index.html`] The webapp used to send baggage via `jaeger-baggage` header, this is now changed to W3C `baggage` header - [`mux.go`] The Jaeger SDK was able to accept `jaeger-baggage` header even for requests without am active trace. OTEL Bridge does not support that, so we use OTEL's Baggage propagator to manually extract the baggage into Context, and once the Bridge creates a Span, we copy OTEL baggage from Context into the Span. - All other changes are cosmetic / logging related --------- Signed-off-by: Yuri Shkuro <github@ysh.us> Signed-off-by: shubbham1215 <sawaikershubham@gmail.com>
jacobmarble
added a commit
to influxdata/influxdb-observability
that referenced
this pull request
Apr 4, 2023
In the demo, the hotrod app was emitting trace events without attributes, including name. It seems that is related to this PR: jaegertracing/jaeger#4187 For now, the easy fix is to rollback hotrod to Jaeger version 1.41.
jacobmarble
added a commit
to influxdata/influxdb-observability
that referenced
this pull request
Apr 4, 2023
In the demo, the hotrod app was emitting trace events without attributes, including name. It seems that is related to this PR: jaegertracing/jaeger#4187 For now, the easy fix is to rollback hotrod to Jaeger version 1.41.
jacobmarble
added a commit
to influxdata/influxdb-observability
that referenced
this pull request
Apr 4, 2023
In the demo, the hotrod app was emitting trace events without attributes, including name. It seems that is related to this PR: jaegertracing/jaeger#4187 For now, the easy fix is to rollback hotrod to Jaeger version 1.41.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on earlier PR #3390 by @rbroggi.
Which problem is this PR solving?
Short description of the changes
Breaking changes
Trace export mode changed from UDP-to-Agent to HTTP-to-Collector.
Most of the environment variables recognized by the Jaeger SDK are no longer supported.
Instead, use environment variables understood by the OpenTelemetry exporters, e.g. names as follows: