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 7 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
13 changes: 13 additions & 0 deletions api/all/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,36 @@ plugins {
id "maven-publish"

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

description = 'OpenTelemetry API'
ext.moduleName = "io.opentelemetry.api"
archivesBaseName = "opentelemetry-api"

testSets {
testLogsIfSdkFound
testDoesNotLogIfSdkFoundAndSuppressed
}

dependencies {
api project(':api:context'),
project(':api:baggage'),
project(':api:common'),
project(':api:trace')

testLogsIfSdkFoundImplementation project(':sdk:all')
testDoesNotLogIfSdkFoundAndSuppressedImplementation project(':sdk:all')

annotationProcessor libraries.auto_value

signature libraries.android_signature

testImplementation libraries.jqf,
libraries.guava_testlib
}

testDoesNotLogIfSdkFoundAndSuppressed {
jvmArgs "-Dotel.sdk.suppress-sdk-initialized-warning=true"
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.spi.OpenTelemetryFactory;
import io.opentelemetry.spi.trace.TracerProviderFactory;
import java.util.logging.Level;
import java.util.logging.Logger;
import javax.annotation.Nullable;

/**
Expand All @@ -30,6 +32,12 @@
* @see ContextPropagators
*/
public final class GlobalOpenTelemetry {
// Visible for testing
static final Logger logger = Logger.getLogger(GlobalOpenTelemetry.class.getName());

private static final boolean suppressSdkCheck =
Boolean.getBoolean("otel.sdk.suppress-sdk-initialized-warning");
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 OpenTelemetry globalOpenTelemetry;

Expand All @@ -45,19 +53,27 @@ private GlobalOpenTelemetry() {}
* interface FQCN but the specified provider cannot be found.
*/
public static OpenTelemetry get() {
if (globalOpenTelemetry == null) {
OpenTelemetry current = globalOpenTelemetry;
if (current == null) {
synchronized (mutex) {
if (globalOpenTelemetry == null) {
current = globalOpenTelemetry;
if (current == null) {
if (!suppressSdkCheck) {
SdkChecker.logIfSdkFound();
}

OpenTelemetryFactory openTelemetryFactory = Utils.loadSpi(OpenTelemetryFactory.class);
if (openTelemetryFactory != null) {
current = openTelemetryFactory.create();
set(openTelemetryFactory.create());
} else {
set(DefaultOpenTelemetry.builder().build());
current = DefaultOpenTelemetry.builder().build();
}
set(current);
}
}
}
return globalOpenTelemetry;
return current;
}

/**
Expand Down Expand Up @@ -115,4 +131,35 @@ public static Tracer getTracer(String instrumentationName, String instrumentatio
public static ContextPropagators getPropagators() {
return get().getPropagators();
}

// 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
}

if (hasSdk) {
// TODO(anuraaga): Play https://www.youtube.com/watch?v=tBA-1ENblN4 if JavaFX detected on
// classpath.
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(), 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.",
new Throwable());
}
}

static void logIfSdkFound() {}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api;

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

import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Handler;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import org.junit.jupiter.api.Test;

public class GlobalOpenTelemetryTest {

@Test
void logIsSuppressed() {
Logger logger = GlobalOpenTelemetry.logger;
AtomicReference<LogRecord> logged = new AtomicReference<>();
Handler handler =
new Handler() {
@Override
public void publish(LogRecord record) {
logged.set(record);
}

@Override
public void flush() {}

@Override
public void close() {}
};
logger.addHandler(handler);
logger.setUseParentHandlers(false);
GlobalOpenTelemetry.get();
assertThat(logged).hasValue(null);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/

package io.opentelemetry.api;

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

import java.util.concurrent.atomic.AtomicReference;
import java.util.logging.Handler;
import java.util.logging.Level;
import java.util.logging.LogRecord;
import java.util.logging.Logger;
import org.junit.jupiter.api.Test;

class GlobalOpenTelemetryTest {

@Test
void logsWarningOnAccessWithoutSdk() {
Logger logger = GlobalOpenTelemetry.logger;
AtomicReference<LogRecord> logged = new AtomicReference<>();
Handler handler =
new Handler() {
@Override
public void publish(LogRecord record) {
logged.set(record);
}

@Override
public void flush() {}

@Override
public void close() {}
};
logger.addHandler(handler);
logger.setUseParentHandlers(false);
GlobalOpenTelemetry.get();
assertThat(logged)
.hasValueSatisfying(
record -> {
assertThat(record.getLevel()).isEqualTo(Level.SEVERE);
assertThat(record.getMessage())
.contains(
"Attempt to access GlobalOpenTelemetry.get before OpenTelemetrySdk has been "
+ "initialized.");
});
}
}