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

Use OpenTelemetry SDK in HotROD #3390

Closed

Conversation

rbroggi
Copy link
Contributor

@rbroggi rbroggi commented Nov 14, 2021

  • replacing the jaeger-client-go ot tracer implementation with the ot-otel bridge implementation

Signed-off-by: rbroggi ro_broggi@hotmail.com

Which problem is this PR solving?

Short description of the changes

  • First iteration in replacing the jaeger client ot tracer implementation with the otel bridge ot implementation

* replacing the jaeger-client-go ot tracer implementation with the ot-otel bridge implementation

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
* replacing the jaeger-client-go ot tracer implementation with the ot-otel bridge implementation

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
@codecov
Copy link

codecov bot commented Nov 14, 2021

Codecov Report

Base: 96.51% // Head: 96.48% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (84e7663) compared to base (9d4d4cd).
Patch has no changes to coverable lines.

❗ Current head 84e7663 differs from pull request most recent head d8f4543. Consider uploading reports for the commit d8f4543 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3390      +/-   ##
==========================================
- Coverage   96.51%   96.48%   -0.03%     
==========================================
  Files         260      260              
  Lines       15215    15215              
==========================================
- Hits        14684    14680       -4     
- Misses        449      452       +3     
- Partials       82       83       +1     
Impacted Files Coverage Δ
pkg/config/tlscfg/cert_watcher.go 92.63% <0.00%> (-2.11%) ⬇️
cmd/query/app/server.go 94.11% <0.00%> (-1.48%) ⬇️
cmd/collector/app/server/zipkin.go 70.73% <0.00%> (+2.43%) ⬆️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

jaeger-ui submodule update should not be in this PR, please make sure to rebase off master

examples/hotrod/pkg/tracing/init.go Outdated Show resolved Hide resolved
* restoring jaeger-ui changes

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
@rbroggi
Copy link
Contributor Author

rbroggi commented Nov 15, 2021

Some comments:

With this change something is not really going as planned as the DAG I get is:

image

instead of the one in the article

Another change that will have to be documented is that we also need to open the grpc port while running the all-in-one container for the go binary to reach jaeger:

docker run -d -p6831:6831/udp -p16686:16686 -p14268:14268 jaegertracing/all-in-one:latest

Will there be a way to update the medium article (it was very useful to me, it would be a shame to loose it)

@rbroggi
Copy link
Contributor Author

rbroggi commented Nov 15, 2021

The frontend tracers are not connecting to the rest:

image

@rbroggi
Copy link
Contributor Author

rbroggi commented Nov 21, 2021

The frontend tracers are not connecting to the rest:

image

Dear @yurishkuro, would you have any advice on how I could troubleshoot this problem efficiently?

By investigating the code I fail to understand why this change had an impact only in the frontend component of the HOTRod. Well I'm not even sure whether the problem lies 100% on the frontend, the only real info I have is that there is a discontinuity between the trace from frontend and the others.

@yurishkuro
Copy link
Member

I suspect this has something to do with the propagator of the context on the wire. What happens when the code calls tracer.Inject() and the tracer is the one from the bridge?

@rbroggi rbroggi force-pushed the 3380-update-hotrod-with-opentel branch from ee92441 to 17df91f Compare November 22, 2021 08:35
rbroggi and others added 9 commits November 22, 2021 17:26
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com>

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
…rtracing#3394)

Bumps [go.opentelemetry.io/collector/model](https://github.com/open-telemetry/opentelemetry-collector) from 0.38.0 to 0.39.0.
- [Release notes](https://github.com/open-telemetry/opentelemetry-collector/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-collector/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-collector@v0.38.0...v0.39.0)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/collector/model
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…ertracing#3400)

* Go back to golang:1.17-alpine

Signed-off-by: Yuri Shkuro <github@ysh.us>

* Add build-base to Docker image

Signed-off-by: Yuri Shkuro <github@ysh.us>
Signed-off-by: Hu Shuai <hus.fnst@fujitsu.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>
* jaeger-ui update
Signed-off-by: rbroggi <ro_broggi@hotmail.com>

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
Signed-off-by: rbroggi <ro_broggi@hotmail.com>

Signed-off-by: rbroggi <ro_broggi@hotmail.com>
@rbroggi rbroggi force-pushed the 3380-update-hotrod-with-opentel branch from 17df91f to d8f4543 Compare November 22, 2021 16:28
@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. You may re-open it if you need more time.

@stale stale bot added the stale The issue/PR has become stale and may be auto-closed label Apr 16, 2022
@stale
Copy link

stale bot commented Apr 30, 2022

This pull request has been automatically closed due to inactivity. You may re-open it if you need more time. We really appreciate your contribution and we are sorry that this has not been completed.

@stale stale bot closed this Apr 30, 2022
@yurishkuro yurishkuro changed the title Issue #3380: update HotROD Use OpenTelemetry SDK in HotROD Apr 30, 2022
@yurishkuro yurishkuro reopened this Jan 28, 2023
@stale stale bot removed the stale The issue/PR has become stale and may be auto-closed label Jan 28, 2023
@yurishkuro yurishkuro closed this Jan 29, 2023
yurishkuro added a commit that referenced this pull request Jan 29, 2023
Based on earlier PR #3390 by @rbroggi. 

## Which problem is this PR solving?
- Resolves #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 #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>
@yurishkuro
Copy link
Member

This was finished in #4187

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 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>
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.

4 participants