Skip to content

GlobalOpenTelemetry.get() should never returns obfuscated Noop OpenTelemetry #7452

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

GlobalOpenTelemetry.get() should never returns obfuscated Noop OpenTelemetry #7452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions api/all/src/main/java/io/opentelemetry/api/GlobalOpenTelemetry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ private GlobalOpenTelemetry() {}
* interface FQCN but the specified provider cannot be found.
*/
public static OpenTelemetry get() {
OpenTelemetry openTelemetry = globalOpenTelemetry;
ObfuscatedOpenTelemetry openTelemetry = globalOpenTelemetry;
if (openTelemetry == null) {
synchronized (mutex) {
openTelemetry = globalOpenTelemetry;
Expand All @@ -88,7 +88,7 @@ public static OpenTelemetry get() {
}
}
}
return openTelemetry;
return openTelemetry.delegate == OpenTelemetry.noop() ? OpenTelemetry.noop() : openTelemetry;
Copy link
Member

Choose a reason for hiding this comment

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

I think we should flip this such that GlobalOpenTelemetry.get() always returns obfuscated opentelemetry, regardless of whether its noop or another instance. Something like:

  public static OpenTelemetry get() {
    OpenTelemetry openTelemetry = globalOpenTelemetry;
    if (openTelemetry == null) {
      synchronized (mutex) {
        openTelemetry = globalOpenTelemetry;
        if (openTelemetry == null) {

          OpenTelemetry autoConfigured = maybeAutoConfigureAndSetGlobal();
          if (autoConfigured != null) {
            return autoConfigured;
          }

          set(OpenTelemetry.noop());
          return Objects.requireNonNull(globalOpenTelemetry);
        }
      }
    }
    return openTelemetry;
  }

Copy link
Author

@quaff quaff Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think we should flip this such that GlobalOpenTelemetry.get() always returns obfuscated opentelemetry, regardless of whether its noop or another instance.

See my comment above, I need to check whether global OpenTelemetry is registered.
I think it's fine to treat noop specially since it's exposed by public API OpenTelemetry.noop().

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I don't think comparison to OpenTelemetry.noop() is the right way to do that.

@open-telemetry/java-approvers - I think we need to be consistent here and obfuscate either ALL or NONE of the responses to calling GlobalOpenTelemetry.get(). And of course the risk with not obfuscating is that API users could cast to OpenTelemetrySdk and call things like shutdown, flush.

We could choose to non obfuscate and have the autoconfigure module obfuscate before calling GlobalOpenTelemetry.set(), but I think this still exposes too much of a foot gun. Too easy to accidentally set GlobalOpenTelemetry to a non-obfuscated version and run into the problems above.

The core thing that @quaff is asking for has come up before: How can I check if GlobalOpenTelemetry is set without causing the side affect of setting it?

If we want to support this, we should add something like a GlobalOpenTelemetry#getIfSet() method, whose response is @Nullable and which is null if set has not been called. This would allow callers to atomically check if set has been called and if so, use the result for whatever they need. We could take a naive approach like adding GlobalOpenTelemetry#isSet, but this has race conditions in it.

The key question here is when would we recommend calling this GlobalOpenTelemetry#getIfSet() method?

The whole point of GlobalOpenTelemetry's current design is that it avoids hard-to-debug initialization questions by ensuring that all calls to GlobalOpenTelemetry.get() get the same result every time. Does adding getIfSet break that? What about the use case @quaff brings up which is essentially: if the agent is present, I want to use the OpenTelemetry instance provided by it, else I want to configure my own instance.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah I don't think comparison to OpenTelemetry.noop() is the right way to do that.

Could you explain why? the instance returned by public API OpenTelemetry.noop() is immutable, it is safe enough.

we should add something like a GlobalOpenTelemetry#getIfSet() method.

I don't like it, we introduce Noop to avoid return null, but it breaks the contract here.

We could take a naive approach like adding GlobalOpenTelemetry#isSet

I don't like it neither, but it's better than GlobalOpenTelemetry#getIfSet().

Copy link
Member

Choose a reason for hiding this comment

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

Could you explain why? the instance returned by public API OpenTelemetry.noop() is immutable, it is safe enough.

I think I don't like the idea of having to provide guidance to users of the form: check if GlobalOpenTelemetry.get() == OpenTelemetry.noop(). It feels weird to have to do that comparison. Also, it gives special meaning / speecial treatment to OpenTelemetry.noop() when I think the more precise question we're trying to answer is "was GlobalOpenTelemetry set"?

On the other hand, the getIfSet() and isSet are ugly because now our guidance becomes more situational:

  • Call GlobalOpenTelemetry.get() if you're ok with the side affect of the call initializing GlobalOpenTelemetry
  • Call getIfSet() / isSet() if you don't want the side affect
    Well, what's our guidance for when you do or do not want the side affect?

I guess I'm softening my stance, but I still wish we could have a more reliable / idiomatic way to signal to the caller that GlobalOpenTelemetry was not set. On the point of a more reliable signal- its totally possible for someone else to provide their own OpenTelemetry implementation that is a noop. We essentially do this here - we configure the OpenTelemetrySdk in such a way that it operates like OpenTelemetry.noop(), but fulfills the full OpenTelemetrySdk contract. And yet this instance of OpenTelemetry is obfuscated and callers of GlobalOpenTelemetry.get() have no way of knowing that its effectively a noop.

We could do something like add a method: boolean OpenTelemetry#isNoop() so that implementations could signal that they are a noop in an unambiguous way.

Copy link
Author

Choose a reason for hiding this comment

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

We could do something like add a method: boolean OpenTelemetry#isNoop() so that implementations could signal that they are a noop in an unambiguous way.

People may set OpenTelemetry.noop() or their own noop implementation, still cannot distinguish whether GlobalTelemetry.set() is called or not.

Copy link
Member

Choose a reason for hiding this comment

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

Well they can't do that with the solution in this PR either.

We'll need to decide whether for use cases like yours, whether: its important to determine that GlobalOpenTelemetry was set, OR that the resolved GlobalOpenTelemetry instance is a noop (remember that GlobalOpenTelemetry.get() is guaranteed to return the same instance for the entire application lifecycle).

Copy link
Author

Choose a reason for hiding this comment

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

whether: its important to determine that GlobalOpenTelemetry was set, OR that the resolved GlobalOpenTelemetry instance is a noop

Why not both?

}

/**
Expand Down
7 changes: 7 additions & 0 deletions ...ternal/src/main/java/io/opentelemetry/api/testing/internal/AbstractOpenTelemetryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,11 @@ void toString_noop_Valid() {
+ "propagators=DefaultContextPropagators{textMapPropagator=NoopTextMapPropagator}"
+ "}");
}

@Test
void neverReturnsObfuscatedNoop() {
assertThat(GlobalOpenTelemetry.get()).isSameAs(OpenTelemetry.noop());
// ensure sequential calls of GlobalOpenTelemetry.get() return same object
assertThat(GlobalOpenTelemetry.get()).isSameAs(OpenTelemetry.noop());
Comment on lines +121 to +123
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that storing the result of OpenTelemetry.noop() as a local variable and using that for both comparisons would be a small improvement on this test.

}
}
2 changes: 1 addition & 1 deletion .../src/test/java/io/opentelemetry/sdk/autoconfigure/AutoConfiguredOpenTelemetrySdkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ void builder_addLoggerProviderCustomizer() {

@Test
void builder_setResultAsGlobalFalse() {
GlobalOpenTelemetry.set(OpenTelemetry.noop());
GlobalOpenTelemetry.set(mock(OpenTelemetry.class));

OpenTelemetrySdk openTelemetry = builder.build().getOpenTelemetrySdk();

Expand Down
3 changes: 2 additions & 1 deletion .../testIncubating/java/io/opentelemetry/sdk/autoconfigure/DeclarativeConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -161,7 +162,7 @@ void configFile_NoShutdownHook() {

@Test
void configFile_setResultAsGlobalFalse() {
GlobalOpenTelemetry.set(OpenTelemetry.noop());
GlobalOpenTelemetry.set(mock(OpenTelemetry.class));
ConfigProperties config =
DefaultConfigProperties.createFromMap(
Collections.singletonMap("otel.experimental.config.file", configFilePath.toString()));
Expand Down
Loading