Log warning if SDK is detected on classpath but has not been initialized when accessing global. #2396

anuraaga · 2020-12-23T03:23:22Z

This is on the read side, whereas #2380 is on the write side. We'll probably want both.

… into globalopentelemetry-check

bogdandrutu

This makes the assumption that we remove SPI, otherwise this is fine to happen. Do we want this if user expects to use SPI?

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

anuraaga · 2020-12-23T03:33:44Z

@bogdandrutu I am proposing having all autoconfiguration in a separate artifact. That artifact could use SPI to allow configuration of non-env var controlled stuff. And this code could be updated to detect whether the artifact (which is under our control so doesn't need a generic entry point mechanism) is on the classpath and call it's initialization entry point with reflection if we want it to be a completely automatic process, and this log wouldn't be thrown for users of that SPI-approach.

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

jkwatson

I approve the removal of SPI from the main API, so I also approve of this as a stop-gap warning to show the user that they need to force their SDK init earlier.

bogdandrutu · 2020-12-24T16:14:51Z

@jkwatson you said you approve removing of the SPI, but SPI is still there. So I am confused what this PR gives us (especially if I use SPI, which is still available).

bogdandrutu · 2020-12-24T16:14:30Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+  static final Logger logger = Logger.getLogger(GlobalOpenTelemetry.class.getName());
+
+  private static final boolean suppressSdkCheck =
+      Boolean.getBoolean("otel.sdk.suppress-sdk-initialized-warning");


Why do we refer to SDK in the API?

Because our SDK is going to be the solution for 99.99% of all users. I think it's fine to refer to our SDK like this.

bogdandrutu · 2020-12-24T16:15:44Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+
+  // Use an inner class that checks and logs in its static initializer to have log-once behavior
+  // using initial classloader lock and no further runtime locks or atomics.
+  private static class SdkChecker {


Call it "ImplementationChecker". We are currently refer as "SDK" to our official implementation.

This is indeed checking for our official implementation so SdkChecker seems correct

bogdandrutu · 2020-12-24T16:16:14Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+    static {
+      boolean hasSdk = false;
+      try {
+        Class.forName("io.opentelemetry.sdk.OpenTelemetrySdk");


Why do we hardcode our implementation in the API artifact?

Bigger question is why not? We have an official implementation, the OpenTelemetry SDK, which we expect to be used in the vast majority of cases. If there's something we get from picking into other artifacts we publish, there isn't really a reason not to. It's a bit like how we have the comments "package-private to allow usage from auto-instrumentation" - that's even another repository, but we think it's worth it to allow good instrumentation experience.

anuraaga

you said you approve removing of the SPI, but SPI is still there. So I am confused what this PR gives us (especially if I use SPI, which is still available).

@bogdandrutu This PR gets us a warning when the global is used before the user initializes the SDK. It doesn't have any downside on the current SPI. And the current SPI will probably be removed after #2401, we've discussed that it isn't helping much in #2371. An API-level SPI for autoloading a custom implementation can be added later if it helps a lot, until we understand better the actual usage patterns of custom implementations it's hard to say. In the meantime, let's focus on the UX of what we expect, the OpenTelemetry SDK.

anuraaga · 2020-12-25T01:59:11Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+
+  // Use an inner class that checks and logs in its static initializer to have log-once behavior
+  // using initial classloader lock and no further runtime locks or atomics.
+  private static class SdkChecker {


This is indeed checking for our official implementation so SdkChecker seems correct

anuraaga · 2020-12-25T02:01:18Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+    static {
+      boolean hasSdk = false;
+      try {
+        Class.forName("io.opentelemetry.sdk.OpenTelemetrySdk");


Bigger question is why not? We have an official implementation, the OpenTelemetry SDK, which we expect to be used in the vast majority of cases. If there's something we get from picking into other artifacts we publish, there isn't really a reason not to. It's a bit like how we have the comments "package-private to allow usage from auto-instrumentation" - that's even another repository, but we think it's worth it to allow good instrumentation experience.

… into globalopentelemetry-check

anuraaga

@jkwatson @bogdandrutu Do you think we can add this in now that the SPI isn't there?

…nto globalopentelemetry-check

anuraaga · 2021-03-05T07:47:50Z

@jkwatson @bogdandrutu I realized that we do need this PR (#2548 (review) was only about warning when set is called in the wrong order, but does not help in the case that buildAndRegisterGlobal is forgotten completely). Can we go ahead and add this?

anuraaga · 2021-03-05T07:48:29Z

For reference: open-telemetry/opentelemetry-java-instrumentation#1550 (comment)

trask

For reference: open-telemetry/opentelemetry-java-instrumentation#1550 (comment)

👍

asarkar · 2021-03-05T08:00:05Z

@anuraaga I suggest that the warning log clearly say what the problem is, and what to do to fix it, like "No OpenTelemetry implementation found; consider adding <Maven GAV> to classpath" instead of get/set stuff which dumb users like me won't understand.

anuraaga · 2021-03-05T08:01:49Z

@asarkar It says Make sure to initialize OpenTelemetrySdk, using OpenTelemetrySdk.builder()...buildAndRegisterGlobal(), as early as possible in your application. Isn't that what you need to know?

asarkar · 2021-03-05T08:04:59Z

It says Make sure to initialize OpenTelemetrySdk, using OpenTelemetrySdk.builder()...buildAndRegisterGlobal(), as early as possible in your application. Isn't that what you need to know?

I'd need to know where to get it. Remember, if someone is missing the SDK, they don't have OpenTelemetrySdk on classpath.

anuraaga · 2021-03-05T08:09:46Z

@asarkar Yeah this message is only thrown when the SDK is on the classpath, where it's almost always the case that it's an error to have forgotten to register it. It makes this PR a relative no-brainer.

This PR does not change the behavior when SDK isn't on the classpath, in which case noop is used - this actually does have real use cases for example, a library may be instrumented with OpenTelemetry but a user doesn't actually want to enable OpenTelemetry in their app by including the SDK. It might still be worth logging something for this case but it's not as clear cut and we can think about it separate from this one.

jkwatson · 2021-03-05T17:37:43Z

I still think this is a good addition. @bogdandrutu any new thoughts?

jkwatson · 2021-03-12T17:17:50Z

I still think this is a good addition. @bogdandrutu any new thoughts?

@bogdandrutu seems like we should unblock and get this merged. You ok with that?

jkwatson · 2021-06-08T22:54:20Z

@anuraaga I think we can unblock this if you are interested.

anuraaga · 2021-06-10T05:29:01Z

@jkwatson Yeah it'd be nice to merge this

ghost · 2021-06-10T09:08:22Z

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java

+                + "OpenTelemetrySdk.builder()...buildAndRegisterGlobal(), as early as possible in "
+                + "your application. If you do not need to use the OpenTelemetry SDK, either "
+                + "exclude it from your classpath or set the "
+                + "'otel.sdk.suppress-sdk-initialized-warning' system property to true.",


Looking at how we name configuration properties enabling/disabling various options here and in the "-instrumentation" I believe that a better one could be otel.sdk.not-initialized-warning.enable set by default to true. In all of the other cases we use "enable" rather that "supress (disable)".

original object is now moot, since SPI has been removed.

jkwatson · 2021-08-16T19:41:05Z

@anuraaga I'm sure this needs a rebase. Thanks!

github-actions · 2022-06-22T21:29:49Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

github-actions · 2022-07-06T22:30:52Z

Closed as inactive. Feel free to reopen if this PR is still being worked on.

Anuraag Agrawal added 7 commits December 22, 2020 13:10

Add guard for accessing global before initialized.

cdcc040

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

2d84255

… into globalopentelemetry-check

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

96a3d82

… into globalopentelemetry-check

Finish

263518a

Optimize

7f453f6

Tests

111f9b0

style

7f6fedd

anuraaga requested review from arminru, bogdandrutu, carlosalberto, jkwatson, Oberon00, pavolloffay, thisthat and tylerbenson as code owners December 23, 2020 03:23

bogdandrutu previously requested changes Dec 23, 2020

View reviewed changes

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Show resolved Hide resolved

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Show resolved Hide resolved

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Outdated Show resolved Hide resolved

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Outdated Show resolved Hide resolved

Comments

311627c

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Show resolved Hide resolved

jkwatson reviewed Dec 23, 2020

View reviewed changes

api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java Show resolved Hide resolved

anuraaga mentioned this pull request Dec 23, 2020

Add autoconfiguration wrapper artifact #2401

Merged

jkwatson approved these changes Dec 23, 2020

View reviewed changes

bogdandrutu reviewed Dec 24, 2020

View reviewed changes

anuraaga commented Dec 25, 2020

View reviewed changes

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

46c36ff

… into globalopentelemetry-check

Invisible

1eeb3c0

anuraaga mentioned this pull request Jan 13, 2021

Remove OpenTelemetry SPI #2504

Merged

Merge branch 'master' of github.com:open-telemetry/opentelemetry-java…

5b6ecff

… into globalopentelemetry-check

anuraaga commented Jan 15, 2021

View reviewed changes

anuraaga mentioned this pull request Jan 20, 2021

Prevent setting global OpenTelemetry twice. #2548

Merged

Base automatically changed from master to main January 26, 2021 00:26

Anuraag Agrawal added 2 commits March 5, 2021 16:43

Merge branch 'main' of github.com:open-telemetry/opentelemetry-java i…

7b33d40

…nto globalopentelemetry-check

Finish

236903b

trask approved these changes Mar 5, 2021

View reviewed changes

anuraaga mentioned this pull request Mar 5, 2021

Kotlin coroutine missing trace open-telemetry/opentelemetry-java-instrumentation#1550

Closed

Extra space.

0f53088

Merge branch 'main' into globalopentelemetry-check

0e47c55

ghost approved these changes Jun 10, 2021

View reviewed changes

jkwatson removed the request for review from tylerbenson April 8, 2022 17:25

github-actions bot added the Stale label Jun 22, 2022

github-actions bot closed this Jul 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log warning if SDK is detected on classpath but has not been initialized when accessing global. #2396

Log warning if SDK is detected on classpath but has not been initialized when accessing global. #2396

anuraaga commented Dec 23, 2020

bogdandrutu left a comment

anuraaga commented Dec 23, 2020

jkwatson left a comment

bogdandrutu commented Dec 24, 2020

bogdandrutu Dec 24, 2020

jkwatson Jan 4, 2021

bogdandrutu Dec 24, 2020

anuraaga Dec 25, 2020

bogdandrutu Dec 24, 2020

anuraaga Dec 25, 2020

anuraaga left a comment

anuraaga Dec 25, 2020

anuraaga Dec 25, 2020

anuraaga left a comment

anuraaga commented Mar 5, 2021

anuraaga commented Mar 5, 2021

trask left a comment

asarkar commented Mar 5, 2021 •

edited

Loading

anuraaga commented Mar 5, 2021

asarkar commented Mar 5, 2021

anuraaga commented Mar 5, 2021

jkwatson commented Mar 5, 2021

jkwatson commented Mar 12, 2021

jkwatson commented Jun 8, 2021

anuraaga commented Jun 10, 2021

ghost Jun 10, 2021

jkwatson commented Aug 16, 2021

github-actions bot commented Jun 22, 2022

github-actions bot commented Jul 6, 2022

Log warning if SDK is detected on classpath but has not been initialized when accessing global. #2396

Log warning if SDK is detected on classpath but has not been initialized when accessing global. #2396

Conversation

anuraaga commented Dec 23, 2020

bogdandrutu left a comment

Choose a reason for hiding this comment

anuraaga commented Dec 23, 2020

jkwatson left a comment

Choose a reason for hiding this comment

bogdandrutu commented Dec 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

anuraaga commented Mar 5, 2021

anuraaga commented Mar 5, 2021

trask left a comment

Choose a reason for hiding this comment

asarkar commented Mar 5, 2021 • edited Loading

anuraaga commented Mar 5, 2021

asarkar commented Mar 5, 2021

anuraaga commented Mar 5, 2021

jkwatson commented Mar 5, 2021

jkwatson commented Mar 12, 2021

jkwatson commented Jun 8, 2021

anuraaga commented Jun 10, 2021

Choose a reason for hiding this comment

jkwatson commented Aug 16, 2021

github-actions bot commented Jun 22, 2022

github-actions bot commented Jul 6, 2022

asarkar commented Mar 5, 2021 •

edited

Loading