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

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

Merged
merged 3 commits into from Jun 20, 2023

Conversation

afzal442
Copy link
Contributor

@afzal442 afzal442 commented Jun 16, 2023

Which problem is this PR solving?

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

Before:
r2

After:
r1

@afzal442
Copy link
Contributor Author

afzal442 commented Jun 16, 2023

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
Signed-off-by: afzal442 <afzal442@gmail.com>
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

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.

Signed-off-by: afzal442 <afzal442@gmail.com>
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
Signed-off-by: afzal442 <afzal442@gmail.com>

refactors ot span error for tracer ctx
@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
33 checks passed
@afzal442 afzal442 deleted the add-otel-tracer branch June 21, 2023 03:55
yurishkuro pushed a commit that referenced this pull request Jul 15, 2023
## 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants