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

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

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
22 changes: 22 additions & 0 deletions api/all/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,40 @@ plugins {
id("maven-publish")

id("me.champeau.jmh")
id("org.unbroken-dome.test-sets")
id("ru.vyarus.animalsniffer")
}

description = "OpenTelemetry API"
extra["moduleName"] = "io.opentelemetry.api"
base.archivesBaseName = "opentelemetry-api"

testSets {
create("testLogsIfSdkFound")
create("testDoesNotLogIfSdkFoundAndSuppressed")
}

dependencies {
api(project(":context"))

annotationProcessor("com.google.auto.value:auto-value")

testImplementation("edu.berkeley.cs.jqf:jqf-fuzz")
testImplementation("com.google.guava:guava-testlib")

add("testLogsIfSdkFoundImplementation", project(":sdk:all"))
add("testDoesNotLogIfSdkFoundAndSuppressedImplementation", project(":sdk:all"))
}

tasks {
val testLogsIfSdkFound by existing(Test::class) {
}

val testDoesNotLogIfSdkFoundAndSuppressed by existing(Test::class) {
jvmArgs("-Dotel.sdk.suppress-sdk-initialized-warning=true")
}

named("check") {
dependsOn(testLogsIfSdkFound, testDoesNotLogIfSdkFoundAndSuppressed)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ public final class GlobalOpenTelemetry {

private static final Logger logger = Logger.getLogger(GlobalOpenTelemetry.class.getName());

private static final boolean suppressSdkCheck =
Boolean.getBoolean("otel.sdk.suppress-sdk-initialized-warning");
anuraaga marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Why do we refer to SDK in the API?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


private static final Object mutex = new Object();

@Nullable private static volatile ObfuscatedOpenTelemetry globalOpenTelemetry;
Expand All @@ -58,6 +61,10 @@ public static OpenTelemetry get() {
return autoConfigured;
}

if (!suppressSdkCheck) {
SdkChecker.logIfSdkFound();
}

set(OpenTelemetry.noop());
return OpenTelemetry.noop();
}
Expand Down Expand Up @@ -168,6 +175,39 @@ private static OpenTelemetry maybeAutoConfigure() {
}
}

// 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

Why do we hardcode our implementation in the API artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

hasSdk = true;
} catch (Throwable t) {
// Ignore
anuraaga marked this conversation as resolved.
Show resolved Hide resolved
}

if (hasSdk) {
logger.log(
Level.SEVERE,
"Attempt to access GlobalOpenTelemetry.get before OpenTelemetrySdk has been "
+ "initialized. This generally means telemetry will not be recorded for parts of "
+ "your application. Make sure to initialize OpenTelemetrySdk, using "
+ "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.",
Copy link

Choose a reason for hiding this comment

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

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)".

// Add stack trace to log to allow user to find the problematic invocation.
new Throwable());
jkwatson marked this conversation as resolved.
Show resolved Hide resolved
}
}

// All the logic is in the static initializer, this method is called just to load the class and
// that's it. JVM will then optimize it away completely because it's empty so we have no
// overhead for a log-once pattern.
static void logIfSdkFound() {}
jkwatson marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Static global instances are obfuscated when they are returned from the API to prevent users
* from casting them to their SDK-specific implementation. For example, we do not want users to
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api;

import io.github.netmikey.logunit.api.LogCapturer;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

public class GlobalOpenTelemetryTest {

@RegisterExtension
LogCapturer logs = LogCapturer.create().captureForType(GlobalOpenTelemetry.class);

@Test
void logIsSuppressed() {
GlobalOpenTelemetry.get();
logs.assertDoesNotContain(
"Attempt to access GlobalOpenTelemetry.get before OpenTelemetrySdk has been "
+ "initialized.");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api;

import static org.assertj.core.api.Assertions.assertThat;

import io.github.netmikey.logunit.api.LogCapturer;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.slf4j.event.Level;
import org.slf4j.event.LoggingEvent;

class GlobalOpenTelemetryTest {

@RegisterExtension
LogCapturer logs = LogCapturer.create().captureForType(GlobalOpenTelemetry.class);

@Test
void logsWarningOnAccessWithoutSdk() {
GlobalOpenTelemetry.get();
LoggingEvent log =
logs.assertContains(
"Attempt to access GlobalOpenTelemetry.get before OpenTelemetrySdk has been "
+ "initialized.");
assertThat(log.getLevel()).isEqualTo(Level.ERROR);
}
}