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.

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

Jump to bottom

Expose AutoConfiguredOpenTelemetrySdk to AgentListener #4831

Merged
merged 3 commits into from
Dec 10, 2021
Merged

Expose AutoConfiguredOpenTelemetrySdk to AgentListener #4831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f379f70
Expose AutoConfiguredOpenTelemetrySdk to AgentListener
jack-berg Dec 7, 2021
4ac5d51
Only call AgentListener if noop is disabled, deprecate AgentListener …
jack-berg Dec 9, 2021
d5984b6
Call AgentListener in DelayedAfterAgentCallback
jack-berg Dec 10, 2021
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
2 changes: 2 additions & 0 deletions javaagent-extension-api/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ dependencies {

// metrics are unstable, do not expose as api
implementation("io.opentelemetry:opentelemetry-sdk-metrics")
// autoconfigure is unstable, do not expose as api
implementation("io.opentelemetry:opentelemetry-sdk-extension-autoconfigure")
}
41 changes: 34 additions & 7 deletions ...agent-extension-api/src/main/java/io/opentelemetry/javaagent/extension/AgentListener.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,59 @@
import io.opentelemetry.instrumentation.api.cache.Cache;
import io.opentelemetry.instrumentation.api.config.Config;
import io.opentelemetry.instrumentation.api.field.VirtualField;
import io.opentelemetry.sdk.autoconfigure.AutoConfiguredOpenTelemetrySdk;
import java.lang.instrument.Instrumentation;
import net.bytebuddy.agent.builder.AgentBuilder;

/**
* {@link AgentListener} can be used to execute code before/after Java agent installation, for
* example to install any implementation providers that are used by instrumentations. For instance,
* this project uses this SPI to install OpenTelemetry SDK.
* example to install any implementation providers that are used by instrumentations. Can also be
* used to obtain the {@link AutoConfiguredOpenTelemetrySdk}.
*
* <p>This is a service provider interface that requires implementations to be registered in a
* provider-configuration file stored in the {@code META-INF/services} resource directory.
*/
public interface AgentListener extends Ordered {

/**
* Runs before the {@link AgentBuilder} construction, before any instrumentation is added.
* Runs before {@link AgentBuilder} construction, before any instrumentation is added.
*
* <p>Execute only a minimal code because any classes loaded before the agent installation will
* have to be retransformed, which takes extra time, and more importantly means that fields can't
* be added to those classes - which causes {@link VirtualField} to fall back to the less
* performant {@link Cache} implementation for those classes.
* <p>Execute only minimal code because any classes loaded before the agent installation will have
* to be retransformed, which takes extra time, and more importantly means that fields can't be
* added to those classes - which causes {@link VirtualField} to fall back to the less performant
* {@link Cache} implementation for those classes.
*
* @deprecated Use {@link #beforeAgent(Config, AutoConfiguredOpenTelemetrySdk)}
*/
@Deprecated
default void beforeAgent(Config config) {}
Copy link
Member Author

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 access AutoConfiguredOpenTelemetrySdk#getConfig().

Copy link
Member

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 finding noop useful?

Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

If we continue supporting noop, how would we remove the config arg once the config problem is solved? since in the noop case AutoConfiguredOpenTelemetrySdk is null, so it seems like we still need to pass around agent configuration somehow

Copy link
Contributor

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?

Copy link
Member

Choose a reason for hiding this comment

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

sound good 👍

Copy link
Member Author

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 call AgentListener methods if the agent is enabled and the sdk is installed? Is there a use case for having the AgentListener when noop is installed? If we did that, we could define the AgentListener as:

interface AgentListener {
  
  default void beforeAgent(Config config, AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
    beforeAgent(config);
  }

  @Deprecated
  default void beforeAgent(Config config) {}
  
  default void afterAgent(Config config, AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
    afterAgent(config);
  }

  @Deprecated
  default void afterAgent(Config config) {}
}

I prefer having access to AutoConfiguredOpenTelemetrySdk versus OpenTelemetry and Resource because:

  • We have access to OpenTelemetrySdk instead of OpenTelemetry, which gives access to SdkTracerProvider and SdkMeterProvider instead of just TracerProvider and MeterProvider. This also means that we could give access to SdkLogEmitterProvider which will probably never be accessible via OpenTelemetry.
  • We give the user access to holder of all the autoconfigured things, which means that as the set of autoconfigured things changes, the user already access without further API changes.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a use case for having the AgentListener when noop is installed?

this is a good question. I'm guessing ideally we would still install things like RuntimeMetricsInstaller in the noop case, but maybe we can live without that?

Copy link
Member

Choose a reason for hiding this comment

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

I prefer having access to AutoConfiguredOpenTelemetrySdk versus OpenTelemetry and Resource because:

  • We have access to OpenTelemetrySdk instead of OpenTelemetry, which gives access to SdkTracerProvider and SdkMeterProvider instead of just TracerProvider and MeterProvider. This also means that we could give access to SdkLogEmitterProvider which will probably never be accessible via OpenTelemetry.
  • We give the user access to holder of all the autoconfigured things, which means that as the set of autoconfigured things changes, the user already access without further API changes.

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.

Copy link
Member Author

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?


/**
* Runs before {@link AgentBuilder} construction, before any instrumentation is added. Not called
* if noop api enabled via {@code otel.javaagent.experimental.use-noop-api}.
*
* <p>Execute only minimal code because any classes loaded before the agent installation will have
* to be retransformed, which takes extra time, and more importantly means that fields can't be
* added to those classes - which causes {@link VirtualField} to fall back to the less performant
* {@link Cache} implementation for those classes.
*/
default void beforeAgent(
Config config, AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {}

/**
* Runs after instrumentations are added to {@link AgentBuilder} and after the agent is installed
* on an {@link Instrumentation}.
*
* @deprecated Use {@link #afterAgent(Config, AutoConfiguredOpenTelemetrySdk)}
*/
@Deprecated
default void afterAgent(Config config) {}

/**
* Runs after instrumentations are added to {@link AgentBuilder} and after the agent is installed
* on an {@link Instrumentation}. Not called if noop api enabled via {@code
* otel.javaagent.experimental.use-noop-api}.
*/
default void afterAgent(
Config config, AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {}
}
48 changes: 41 additions & 7 deletions javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/AgentInstaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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;
}

Expand All @@ -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);
}
}

Expand All @@ -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).
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

The afterAgent() listener method is also executed from the DelayedAfterAgentCallback (under some specific conditions), can you pass the autoConfiguredSdk there too?

}
}
}
Expand Down Expand Up @@ -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
Expand All @@ -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);
}
Expand Down
4 changes: 3 additions & 1 deletion ...oling/src/main/java/io/opentelemetry/javaagent/tooling/AgentTracerProviderConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@

package io.opentelemetry.javaagent.tooling;

import static io.opentelemetry.javaagent.tooling.AgentInstaller.JAVAAGENT_ENABLED_CONFIG;

import com.google.auto.service.AutoService;
import io.opentelemetry.exporter.logging.LoggingSpanExporter;
import io.opentelemetry.instrumentation.api.config.Config;
Expand All @@ -24,7 +26,7 @@ public class AgentTracerProviderConfigurer implements SdkTracerProviderConfigure
@Override
public void configure(
SdkTracerProviderBuilder sdkTracerProviderBuilder, ConfigProperties config) {
if (!Config.get().getBoolean(OpenTelemetryInstaller.JAVAAGENT_ENABLED_CONFIG, true)) {
if (!Config.get().getBoolean(JAVAAGENT_ENABLED_CONFIG, true)) {
return;
}

Expand Down
92 changes: 35 additions & 57 deletions ...gent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/OpenTelemetryInstaller.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Member Author

Choose a reason for hiding this comment

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

This constant was a duplicate of of AgentInstaller#JAVAAGENT_ENABLED_CONFIG.

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;
}
}
35 changes: 3 additions & 32 deletions ...ling/src/test/groovy/io/opentelemetry/javaagent/tooling/OpenTelemetryInstallerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import spock.lang.Specification

class OpenTelemetryInstallerTest extends Specification {


void setup() {
GlobalOpenTelemetry.resetForTest()
}
Expand All @@ -21,40 +20,12 @@ class OpenTelemetryInstallerTest extends Specification {
GlobalOpenTelemetry.resetForTest()
}


def "should initialize noop"() {

given:
def config = Config.builder()
.readProperties([
(OpenTelemetryInstaller.JAVAAGENT_NOOP_CONFIG) : "true",
(OpenTelemetryInstaller.JAVAAGENT_ENABLED_CONFIG): "true"
])
.build()

when:
def otelInstaller = new OpenTelemetryInstaller()
otelInstaller.beforeAgent(config)

then:
GlobalOpenTelemetry.getTracerProvider() == NoopOpenTelemetry.getInstance().getTracerProvider()
}

def "should NOT initialize noop"() {

given:
def config = Config.builder()
.readProperties([
(OpenTelemetryInstaller.JAVAAGENT_NOOP_CONFIG) : "true",
(OpenTelemetryInstaller.JAVAAGENT_ENABLED_CONFIG): "false"
])
.build()

def "should initialize GlobalOpenTelemetry"() {
when:
def otelInstaller = new OpenTelemetryInstaller()
otelInstaller.beforeAgent(config)
def otelInstaller = OpenTelemetryInstaller.installOpenTelemetrySdk(Config.builder().build())

then:
otelInstaller != null
GlobalOpenTelemetry.getTracerProvider() != NoopOpenTelemetry.getInstance().getTracerProvider()
}

Expand Down