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

instrumentation: fix resource creation, use parent-based sampler. #233

Merged
merged 3 commits into from
Jan 15, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Jan 15, 2024

This patch fixes a few annoyances in our current OpenTelemetry instrumentation code. In particular, it

  • drops support for the discontinued Jaeger-specific exporter
  • avoid using resource.Merge() for creating our otel resource
  • uses a parent-based probabilistic sampler to span sampling

@askervin
Copy link
Collaborator

Just a small typo: s/jeager/jaeger, otherwise LGTM

The jaeger-specific exporter was dropped in Opentelemetry
after 1.17.0. Let's drop support for it, too.

Signed-off-by: Krisztian Litkey <[email protected]>
Use resource.New*() to create our OpenTelemetry resource as
opposed to Merge()ing with resource.Default(). This should
avoid hitting the annoying semconv schema snafu
(open-telemetry/opentelemetry-go#4476)
which we'd otherwise hit if/when the default semconv version
is bumped.

Signed-off-by: Krisztian Litkey <[email protected]>
If we have a tracing enabled, we always want to sample
spans whenever
  - our own probabilistic sampler says so, or
  - we have a parent span and it is being sampled

Therefore use a parent-based probabilistic sampler instead
of just a probabilistic one.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub
Copy link
Collaborator Author

klihub commented Jan 15, 2024

Just a small typo: s/jeager/jaeger, otherwise LGTM

Thanks for spotting it. I keep doing that same mistake both in code comments and commit messages. I blame it on Chuck Y.

Copy link
Collaborator

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub merged commit 5d072b5 into containers:main Jan 15, 2024
3 checks passed
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.

3 participants