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

feat: Added OTEL TracerProvider instead of opentracing.Tracer for hotROD example app service (Redis) #4533

Merged
merged 3 commits into from
Jun 20, 2023
Merged

feat: Added OTEL TracerProvider instead of opentracing.Tracer for hotROD example app service (Redis) #4533

merged 3 commits into from
Jun 20, 2023
+37 −26

Conversation

afzal442
Copy link
Contributor

@afzal442 afzal442 commented Jun 16, 2023 •
edited
Loading

Which problem is this PR solving?

  • This adds OTEL tracerProvider and removes OT tracer for Redis service hotROD app

Before:
r2

After:
r1

@afzal442 afzal442 requested a review from a team as a code owner June 16, 2023 17:16
@afzal442
Copy link
Contributor Author

afzal442 commented Jun 16, 2023 •
edited
Loading

Hi there, currently, it is out of parent context maybe due to spanID. I am not sure why. on it.

Update: Redis spans seem to work fine. Previously, it had an issue with instrumenting the layers in a redis service.

@afzal442 afzal442 marked this pull request as draft June 16, 2023 17:28
@afzal442 afzal442 changed the title [WIP] Added OTEL traceProvider instead of opentracing.Tracer for hotROD example app [WIP] Added OTEL TracerProvider instead of opentracing.Tracer for hotROD example app service Jun 17, 2023
@afzal442 afzal442 marked this pull request as ready for review June 17, 2023 08:25
@afzalbin64 @afzal442
init otel sdks
917be4d 
Signed-off-by: afzal442 <afzal442@gmail.com>
@afzal442 afzal442 requested a review from yurishkuro June 17, 2023 11:11
yurishkuro
yurishkuro reviewed Jun 17, 2023
View reviewed changes
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
@yurishkuro
Copy link
Member

Just tested this PR as is - it's a good sign that Redis spans appear to work just fine with the rest of the trace. Should make this easier when we migrate the main Jaeger code base.

@afzal442 afzal442 changed the title [WIP] Added OTEL TracerProvider instead of opentracing.Tracer for hotROD example app service feat: Added OTEL TracerProvider instead of opentracing.Tracer for hotROD example app service (Redis) Jun 18, 2023
@codecov
Copy link

codecov bot commented Jun 18, 2023 •
edited
Loading

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (bfed6e7) 97.05% compared to head (7bae608) 97.07%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #4533      +/-   ##
==========================================
+ Coverage   97.05%   97.07%   +0.01%     
==========================================
  Files         301      301              
  Lines       17817    17817              
==========================================
+ Hits        17292    17295       +3     
+ Misses        421      419       -2     
+ Partials      104      103       -1
Flag Coverage Δ
unittests 97.07% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@afzal442
upgrade init otel
3ec70cb 
Signed-off-by: afzal442 <afzal442@gmail.com>
yurishkuro
yurishkuro reviewed Jun 19, 2023
View reviewed changes
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Show resolved Hide resolved
examples/hotrod/services/driver/redis.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
@afzal442
rmvs opentracing.tracer span context
7bae608 
Signed-off-by: afzal442 <afzal442@gmail.com>

refactors ot span error for tracer ctx
@afzal442 afzal442 requested a review from yurishkuro June 20, 2023 18:21
@yurishkuro
Copy link
Member

I briefly checked the blog post, and I don't think there is an issue with baggage specifically for the redis service, but it may come up in the future changes. So this PR is ok to merge.

@yurishkuro
Copy link
Member

What would be nice is if we had an integration test for HotROD that was validating all the points being made in the blog post.

@yurishkuro yurishkuro merged commit 109b730 into jaegertracing:main Jun 20, 2023
@afzal442 afzal442 deleted the add-otel-tracer branch June 21, 2023 03:55
@afzal442 afzal442 mentioned this pull request Jun 22, 2023
Merged
yurishkuro pushed a commit that referenced this pull request Jul 15, 2023
@afzal442 @afzalbin64
[hotrod] Upgrade HotROD to use OpenTelemetry instrumentation (#4548)
96f7fdc 
## Which problem is this PR solving?
* Part of #3380
* This PR tries to instrument the hotROD app with otel downgrading
opentracing. Related
#4533 (comment)

## Short description of the changes
- Upgrades hotROD application to support `otel tracer`

---------

Signed-off-by: Afzal Ansari <afzal442@gmail.com>
Signed-off-by: afzal442 <afzal442@gmail.com>
Signed-off-by: Afzal <94980910+afzalbin64@users.noreply.github.com>
Co-authored-by: Afzal <94980910+afzalbin64@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@albertteoh albertteoh albertteoh left review comments

@joe-elliott joe-elliott Awaiting requested review from joe-elliott joe-elliott is a code owner automatically assigned from jaegertracing/jaeger-maintainers

@yurishkuro yurishkuro Awaiting requested review from yurishkuro

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@afzal442 @yurishkuro @albertteoh @afzalbin64