-
Notifications
You must be signed in to change notification settings - Fork 848
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
Expose AutoConfiguredOpenTelemetrySdk to AgentListener #4831
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,14 +6,17 @@ | |
package io.opentelemetry.javaagent.tooling; | ||
|
||
import static io.opentelemetry.javaagent.bootstrap.AgentInitializer.isJavaBefore9; | ||
import static io.opentelemetry.javaagent.tooling.OpenTelemetryInstaller.installOpenTelemetrySdk; | ||
import static io.opentelemetry.javaagent.tooling.SafeServiceLoader.load; | ||
import static io.opentelemetry.javaagent.tooling.SafeServiceLoader.loadOrdered; | ||
import static io.opentelemetry.javaagent.tooling.Utils.getResourceName; | ||
import static net.bytebuddy.matcher.ElementMatchers.any; | ||
|
||
import io.opentelemetry.api.GlobalOpenTelemetry; | ||
import io.opentelemetry.context.Context; | ||
import io.opentelemetry.context.ContextStorage; | ||
import io.opentelemetry.context.Scope; | ||
import io.opentelemetry.extension.noopapi.NoopOpenTelemetry; | ||
import io.opentelemetry.instrumentation.api.config.Config; | ||
import io.opentelemetry.javaagent.bootstrap.AgentClassLoader; | ||
import io.opentelemetry.javaagent.bootstrap.BootstrapPackagePrefixesHolder; | ||
|
@@ -31,6 +34,7 @@ | |
import io.opentelemetry.javaagent.tooling.ignore.IgnoredTypesMatcher; | ||
import io.opentelemetry.javaagent.tooling.muzzle.AgentTooling; | ||
import io.opentelemetry.javaagent.tooling.util.Trie; | ||
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk; | ||
import java.lang.instrument.Instrumentation; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
|
@@ -53,7 +57,8 @@ public class AgentInstaller { | |
|
||
private static final Logger logger; | ||
|
||
private static final String JAVAAGENT_ENABLED_CONFIG = "otel.javaagent.enabled"; | ||
static final String JAVAAGENT_ENABLED_CONFIG = "otel.javaagent.enabled"; | ||
static final String JAVAAGENT_NOOP_CONFIG = "otel.javaagent.experimental.use-noop-api"; | ||
|
||
// This property may be set to force synchronous AgentListener#afterAgent() execution: the | ||
// condition for delaying the AgentListener initialization is pretty broad and in case it covers | ||
|
@@ -110,7 +115,19 @@ public static ResettableClassFileTransformer installBytebuddyAgent( | |
|
||
setBootstrapPackages(config); | ||
|
||
runBeforeAgentListeners(agentListeners, config); | ||
// If noop OpenTelemetry is enabled, autoConfiguredSdk will be null and AgentListeners are not | ||
// called | ||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = null; | ||
if (config.getBoolean(JAVAAGENT_NOOP_CONFIG, false)) { | ||
logger.info("Tracing and metrics are disabled because noop is enabled."); | ||
GlobalOpenTelemetry.set(NoopOpenTelemetry.getInstance()); | ||
} else { | ||
autoConfiguredSdk = installOpenTelemetrySdk(config); | ||
} | ||
|
||
if (autoConfiguredSdk != null) { | ||
runBeforeAgentListeners(agentListeners, config, autoConfiguredSdk); | ||
} | ||
|
||
AgentBuilder agentBuilder = | ||
new AgentBuilder.Default() | ||
|
@@ -157,7 +174,11 @@ public static ResettableClassFileTransformer installBytebuddyAgent( | |
|
||
ResettableClassFileTransformer resettableClassFileTransformer = agentBuilder.installOn(inst); | ||
ClassFileTransformerHolder.setClassFileTransformer(resettableClassFileTransformer); | ||
runAfterAgentListeners(agentListeners, config); | ||
|
||
if (autoConfiguredSdk != null) { | ||
runAfterAgentListeners(agentListeners, config, autoConfiguredSdk); | ||
} | ||
|
||
return resettableClassFileTransformer; | ||
} | ||
|
||
|
@@ -178,9 +199,12 @@ private static void setBootstrapPackages(Config config) { | |
} | ||
|
||
private static void runBeforeAgentListeners( | ||
Iterable<AgentListener> agentListeners, Config config) { | ||
Iterable<AgentListener> agentListeners, | ||
Config config, | ||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) { | ||
for (AgentListener agentListener : agentListeners) { | ||
agentListener.beforeAgent(config); | ||
agentListener.beforeAgent(config, autoConfiguredSdk); | ||
} | ||
} | ||
|
||
|
@@ -199,7 +223,9 @@ private static AgentBuilder configureIgnoredTypes(Config config, AgentBuilder ag | |
} | ||
|
||
private static void runAfterAgentListeners( | ||
Iterable<AgentListener> agentListeners, Config config) { | ||
Iterable<AgentListener> agentListeners, | ||
Config config, | ||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) { | ||
// java.util.logging.LogManager maintains a final static LogManager, which is created during | ||
// class initialization. Some AgentListener implementations may use JRE bootstrap classes | ||
// which touch this class (e.g. JFR classes or some MBeans). | ||
|
@@ -223,10 +249,12 @@ && isJavaBefore9() | |
&& isAppUsingCustomLogManager()) { | ||
logger.debug("Custom JUL LogManager detected: delaying AgentListener#afterAgent() calls"); | ||
registerClassLoadCallback( | ||
"java.util.logging.LogManager", new DelayedAfterAgentCallback(config, agentListeners)); | ||
"java.util.logging.LogManager", | ||
new DelayedAfterAgentCallback(config, agentListeners, autoConfiguredSdk)); | ||
} else { | ||
for (AgentListener agentListener : agentListeners) { | ||
agentListener.afterAgent(config); | ||
agentListener.afterAgent(config, autoConfiguredSdk); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
} | ||
} | ||
} | ||
|
@@ -326,10 +354,15 @@ public static void registerClassLoadCallback(String className, Runnable callback | |
private static class DelayedAfterAgentCallback implements Runnable { | ||
private final Iterable<AgentListener> agentListeners; | ||
private final Config config; | ||
private final AutoConfiguredOpenTelemetrySdk autoConfiguredSdk; | ||
|
||
private DelayedAfterAgentCallback(Config config, Iterable<AgentListener> agentListeners) { | ||
private DelayedAfterAgentCallback( | ||
Config config, | ||
Iterable<AgentListener> agentListeners, | ||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) { | ||
this.agentListeners = agentListeners; | ||
this.config = config; | ||
this.autoConfiguredSdk = autoConfiguredSdk; | ||
} | ||
|
||
@Override | ||
|
@@ -349,6 +382,7 @@ private void runAgentListeners() { | |
for (AgentListener agentListener : agentListeners) { | ||
try { | ||
agentListener.afterAgent(config); | ||
agentListener.afterAgent(config, autoConfiguredSdk); | ||
} catch (RuntimeException e) { | ||
logger.error("Failed to execute {}", agentListener.getClass().getName(), e); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,79 +5,57 @@ | |
|
||
package io.opentelemetry.javaagent.tooling; | ||
|
||
import com.google.auto.service.AutoService; | ||
import io.opentelemetry.api.GlobalOpenTelemetry; | ||
import io.opentelemetry.api.metrics.GlobalMeterProvider; | ||
import io.opentelemetry.api.metrics.MeterProvider; | ||
import io.opentelemetry.extension.noopapi.NoopOpenTelemetry; | ||
import io.opentelemetry.instrumentation.api.config.Config; | ||
import io.opentelemetry.javaagent.bootstrap.AgentInitializer; | ||
import io.opentelemetry.javaagent.extension.AgentListener; | ||
import io.opentelemetry.javaagent.instrumentation.api.OpenTelemetrySdkAccess; | ||
import io.opentelemetry.sdk.OpenTelemetrySdk; | ||
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk; | ||
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdkBuilder; | ||
import io.opentelemetry.sdk.common.CompletableResultCode; | ||
import io.opentelemetry.sdk.metrics.SdkMeterProvider; | ||
import java.util.Arrays; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
@AutoService(AgentListener.class) | ||
public class OpenTelemetryInstaller implements AgentListener { | ||
private static final Logger logger = LoggerFactory.getLogger(OpenTelemetryInstaller.class); | ||
|
||
static final String JAVAAGENT_ENABLED_CONFIG = "otel.javaagent.enabled"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This constant was a duplicate of of |
||
static final String JAVAAGENT_NOOP_CONFIG = "otel.javaagent.experimental.use-noop-api"; | ||
|
||
@Override | ||
public void beforeAgent(Config config) { | ||
installAgentTracer(config); | ||
} | ||
public class OpenTelemetryInstaller { | ||
|
||
/** | ||
* Register agent tracer if no agent tracer is already registered. | ||
* Install the {@link OpenTelemetrySdk} using autoconfigure, and return the {@link | ||
* AutoConfiguredOpenTelemetrySdk}. | ||
* | ||
* @param config Configuration instance | ||
* @return the {@link AutoConfiguredOpenTelemetrySdk} | ||
*/ | ||
@SuppressWarnings("unused") | ||
public static synchronized void installAgentTracer(Config config) { | ||
if (config.getBoolean(JAVAAGENT_ENABLED_CONFIG, true)) { | ||
|
||
if (config.getBoolean(JAVAAGENT_NOOP_CONFIG, false)) { | ||
GlobalOpenTelemetry.set(NoopOpenTelemetry.getInstance()); | ||
} else { | ||
System.setProperty("io.opentelemetry.context.contextStorageProvider", "default"); | ||
|
||
AutoConfiguredOpenTelemetrySdkBuilder builder = | ||
AutoConfiguredOpenTelemetrySdk.builder() | ||
.setResultAsGlobal(true) | ||
.addPropertiesSupplier(config::getAllProperties); | ||
|
||
ClassLoader classLoader = AgentInitializer.getExtensionsClassLoader(); | ||
if (classLoader != null) { | ||
// May be null in unit tests. | ||
builder.setServiceClassLoader(classLoader); | ||
} | ||
|
||
OpenTelemetrySdk sdk = builder.build().getOpenTelemetrySdk(); | ||
OpenTelemetrySdkAccess.internalSetForceFlush( | ||
(timeout, unit) -> { | ||
CompletableResultCode traceResult = sdk.getSdkTracerProvider().forceFlush(); | ||
MeterProvider meterProvider = GlobalMeterProvider.get(); | ||
final CompletableResultCode metricsResult; | ||
if (meterProvider instanceof SdkMeterProvider) { | ||
metricsResult = ((SdkMeterProvider) meterProvider).forceFlush(); | ||
} else { | ||
metricsResult = CompletableResultCode.ofSuccess(); | ||
} | ||
CompletableResultCode.ofAll(Arrays.asList(traceResult, metricsResult)) | ||
.join(timeout, unit); | ||
}); | ||
} | ||
|
||
} else { | ||
logger.info("Tracing is disabled."); | ||
static AutoConfiguredOpenTelemetrySdk installOpenTelemetrySdk(Config config) { | ||
System.setProperty("io.opentelemetry.context.contextStorageProvider", "default"); | ||
|
||
AutoConfiguredOpenTelemetrySdkBuilder builder = | ||
AutoConfiguredOpenTelemetrySdk.builder() | ||
.setResultAsGlobal(true) | ||
.addPropertiesSupplier(config::getAllProperties); | ||
|
||
ClassLoader classLoader = AgentInitializer.getExtensionsClassLoader(); | ||
if (classLoader != null) { | ||
// May be null in unit tests. | ||
builder.setServiceClassLoader(classLoader); | ||
} | ||
|
||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = builder.build(); | ||
OpenTelemetrySdk sdk = autoConfiguredSdk.getOpenTelemetrySdk(); | ||
|
||
OpenTelemetrySdkAccess.internalSetForceFlush( | ||
(timeout, unit) -> { | ||
CompletableResultCode traceResult = sdk.getSdkTracerProvider().forceFlush(); | ||
MeterProvider meterProvider = GlobalMeterProvider.get(); | ||
final CompletableResultCode metricsResult; | ||
if (meterProvider instanceof SdkMeterProvider) { | ||
metricsResult = ((SdkMeterProvider) meterProvider).forceFlush(); | ||
} else { | ||
metricsResult = CompletableResultCode.ofSuccess(); | ||
} | ||
CompletableResultCode.ofAll(Arrays.asList(traceResult, metricsResult)) | ||
.join(timeout, unit); | ||
}); | ||
|
||
return autoConfiguredSdk; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered deprecating this but
beforeAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk)
doesn't get called when there is a noop OpenTelemetry installed. If we think its ok to not have any agent listener hooks when noop is used, then I can go ahead and deprecate this because folks can accessAutoConfiguredOpenTelemetrySdk#getConfig()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be really nice to deprecate this. not sure what to do about
noop
. @jkwatson @breedx-splk are you findingnoop
useful?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think the noop is useful.
Over in the original issue in the other repo we chatted about the two arg
(config, sdk)
version being preferred. This two-version approach requires the SPI user to override both and hold state if they need both. It also looks weird (to me) at the call site below, because, depending on the implementation,beforeAgent()
can be called twice.My preference would be to just have the two-arg version and then later when the config problem (Config vs. ConfigProperties) is solved we can remove the config arg.
How's that sound?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we continue supporting
noop
, how would we remove the config arg once the config problem is solved? since in thenoop
caseAutoConfiguredOpenTelemetrySdk
is null, so it seems like we still need to pass around agent configuration somehowThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a noop implementation equivalent of the
AutoConfiguredOpenTelemetrySdk
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sound good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we keep
noop
, but only callAgentListener
methods if the agent is enabled and the sdk is installed? Is there a use case for having theAgentListener
whennoop
is installed? If we did that, we could define theAgentListener
as:I prefer having access to
AutoConfiguredOpenTelemetrySdk
versusOpenTelemetry
andResource
because:OpenTelemetrySdk
instead ofOpenTelemetry
, which gives access toSdkTracerProvider
andSdkMeterProvider
instead of justTracerProvider
andMeterProvider
. This also means that we could give access toSdkLogEmitterProvider
which will probably never be accessible viaOpenTelemetry
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good question. I'm guessing ideally we would still install things like
RuntimeMetricsInstaller
in thenoop
case, but maybe we can live without that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these are all perfectly valid reasons. Okay, consider me convinced -- I suppose we can live without running a few listeners in case of
noop
; all bytecode manipulations are still done, we're just not installing a couple of metrics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@trask if noop
OpenTelemetry
is installed then the runtime metrics won't have any affect, right?