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

AutoConfiguredOpenTelemetrySdkBuilder does not set GlobalOpenTelemetry by default #5564

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

jack-berg
Copy link
Member

GlobalOpenTelemetry makes you opt into initializing with autoconfigure by setting a environment variable / system property.

In contrast, AutoConfiguredOpenTelemetrySdk will set GlobalOpenTelemetry by default. This PR aligns AutoConfiguredOpenTelemetrySdk with the opt-in behavior of GlobalOpenTelemetry, and also with our general advice discouraging use of GlobalOpenTelemetry.

Also, this PR changes AutoConfiguredOpenTelemetrySdkBuilder#setResultAsGlobal(boolean) to simply be AutoConfiguredOpenTelemetrySdkBuilder#setResultAsGlobal(). This means there is no option to undo a decision to setResultAsGlobal after its previously been invoked.

This aligns with our builders elsewhere.

@jack-berg jack-berg requested a review from a team June 22, 2023 15:43
@codecov
Copy link

codecov bot commented Jun 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03 ⚠️

Comparison is base (cb6d713) 91.40% compared to head (eba323c) 91.38%.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5564      +/-   ##
============================================
- Coverage     91.40%   91.38%   -0.03%     
+ Complexity     4963     4961       -2     
============================================
  Files           550      550              
  Lines         14569    14569              
  Branches       1358     1358              
============================================
- Hits          13317    13314       -3     
- Misses          866      868       +2     
- Partials        386      387       +1     
Impacted Files Coverage Δ
.../autoconfigure/AutoConfiguredOpenTelemetrySdk.java 100.00% <100.00%> (ø)
...nfigure/AutoConfiguredOpenTelemetrySdkBuilder.java 94.40% <100.00%> (-1.87%) ⬇️

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

Copy link
Contributor

@jkwatson jkwatson left a comment

Choose a reason for hiding this comment

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

👍🏽

@gabrieljones
Copy link

@jack-berg
https://opentelemetry.io/docs/concepts/instrumentation/libraries/#getting-a-tracer
This does the opposite of discouraging use of GlobalOpenTelemetry.

@jack-berg
Copy link
Member Author

Yikes @gabrieljones. I'll open an issue / PR to fix that.

@jack-berg
Copy link
Member Author

See issue open-telemetry/opentelemetry.io#3692.

@gabrieljones
Copy link

gabrieljones commented Dec 21, 2023

Edit:
Oh I see, the warning is triggered because something somewhere is calling GlobalOpentelemetry.get()

If I get rid of all of those then the warning should go away.

Original:
Hmmm, the

AutoConfiguredOpenTelemetrySdk found on classpath but automatic configuration is disabled.

warning also seems to "encourage" the use of GlobalOpenTelemetry.

Attempting to resolve this warning led me into a head on collision with the advice not to use GlobalOpenTelemetry.

Is there a way to suppress this warning so that the users of my app are not spooked.

Right now I do:

    val openTelemetry: OpenTelemetry = AutoConfiguredOpenTelemetrySdk.builder.build().getOpenTelemetrySdk

Is there a more correct way to retrieve a non Global instance of OpenTelemetry from AutoConfiguredOpenTelemetrySdk so that the warning is not triggered?

@jack-berg
Copy link
Member Author

Oh I see, the warning is triggered because something somewhere is calling GlobalOpentelemetry.get()
If I get rid of all of those then the warning should go away.

Yup - maybe we should log something more clear / actionable 🤔

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