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

chore: update jaeger remote sampler example #4892

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hcelaloner
Copy link
Contributor

chore: update jaeger remote sampler example using the latest otelcol-contrib extension

Previously, the example was outdated and not working properly. This commit updates the example provided using otelcol-contrib jaegerremotesampling extension.

@hcelaloner hcelaloner requested review from yurishkuro and a team as code owners February 5, 2024 09:40
@hcelaloner hcelaloner force-pushed the chore/update-jaegerremote-example branch from 009fc0b to 27a499a Compare February 6, 2024 07:55
CHANGELOG.md Outdated
@@ -21,6 +21,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Fixed

- Do not panic in `go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc` if `MeterProvider` returns a `nil` instrument. (#4875)
- OpenTelemetry Collector based example for `go.opentelemetry.io/contrib/samplers/jaegerremote`. (#4892)
Copy link
Member

Choose a reason for hiding this comment

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

A Fixed entry would be a bugfix, which this isn't. How about moving it to the Changed section?
Also, how about something along the lines of:

Updated jaeger version in go.opentelemetry.io/contrib/samplers/jaegerremote/example example. (#4892)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the feedback. I also had some doubts about where should I add it while adding it to changelog. Of course 👍 I am going to move it in a minute.

…or extension

Previously, example was outdated and not working properly. This commit updates the example
provided using otelcol-contrib jaegerremotesampling extension.
@hcelaloner hcelaloner force-pushed the chore/update-jaegerremote-example branch from 27a499a to 7c6e086 Compare February 6, 2024 09:29
samplers/jaegerremote/example/main.go Show resolved Hide resolved
@@ -27,12 +30,19 @@ import (
)

func main() {
// Optional: an implementation of logr.Logger used for demo purposes to catch potential error logs
logger := stdr.NewWithOptions(stdlog.New(os.Stderr, "", stdlog.LstdFlags), stdr.Options{LogCaller: stdr.All})
Copy link
Member

Choose a reason for hiding this comment

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

The addition of this logger is also more than just an update. Wouldn't that be adding quite a bit of noise in the output, making the example less readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I wanted to try the example, I couldn't see any error logs from the background updater. By default sampler was using logr.Discard() if I am not wrong, I just added to a simple stdout logr implementation to be able to check error logs while trying. I didn't remove it because I thought that it could be a nice for example.

CHANGELOG.md Outdated Show resolved Hide resolved
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

3 participants