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 1 commit
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
12 changes: 10 additions & 2 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 @@ -29,7 +29,10 @@ public interface AgentListener extends Ordered {
* 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?


/**
Expand All @@ -41,18 +44,23 @@ default void beforeAgent(Config config) {}
* added to those classes - which causes {@link VirtualField} to fall back to the less performant
* {@link Cache} implementation for those classes.
*/
default void beforeAgent(AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {}
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(AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {}
default void afterAgent(
Config config, AutoConfiguredOpenTelemetrySdk autoConfiguredOpenTelemetrySdk) {}
}
23 changes: 14 additions & 9 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 @@ -56,6 +56,7 @@ public class AgentInstaller {
private static final Logger logger;

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 @@ -112,9 +113,13 @@ public static ResettableClassFileTransformer installBytebuddyAgent(

setBootstrapPackages(config);

AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = installOpenTelemetrySdk(config);
boolean enableNoopApi = config.getBoolean(JAVAAGENT_NOOP_CONFIG, false);
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk =
installOpenTelemetrySdk(enableNoopApi, config);

runBeforeAgentListeners(agentListeners, config, autoConfiguredSdk);
if (!enableNoopApi) {
runBeforeAgentListeners(agentListeners, config, autoConfiguredSdk);
}

AgentBuilder agentBuilder =
new AgentBuilder.Default()
Expand Down Expand Up @@ -161,7 +166,11 @@ public static ResettableClassFileTransformer installBytebuddyAgent(

ResettableClassFileTransformer resettableClassFileTransformer = agentBuilder.installOn(inst);
ClassFileTransformerHolder.setClassFileTransformer(resettableClassFileTransformer);
runAfterAgentListeners(agentListeners, config, autoConfiguredSdk);

if (!enableNoopApi) {
runAfterAgentListeners(agentListeners, config, autoConfiguredSdk);
}

return resettableClassFileTransformer;
}

Expand All @@ -187,9 +196,7 @@ private static void runBeforeAgentListeners(
@Nullable AutoConfiguredOpenTelemetrySdk autoConfiguredSdk) {
for (AgentListener agentListener : agentListeners) {
agentListener.beforeAgent(config);
if (autoConfiguredSdk != null) {
agentListener.beforeAgent(autoConfiguredSdk);
}
agentListener.beforeAgent(config, autoConfiguredSdk);
}
}

Expand Down Expand Up @@ -238,9 +245,7 @@ && isAppUsingCustomLogManager()) {
} else {
for (AgentListener agentListener : agentListeners) {
agentListener.afterAgent(config);
if (autoConfiguredSdk != null) {
agentListener.afterAgent(autoConfiguredSdk);
}
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
10 changes: 4 additions & 6 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 @@ -25,18 +25,16 @@
public class OpenTelemetryInstaller {
private static final Logger logger = LoggerFactory.getLogger(OpenTelemetryInstaller.class);

static final String JAVAAGENT_NOOP_CONFIG = "otel.javaagent.experimental.use-noop-api";

/**
* Install the {@link OpenTelemetrySdk} using autoconfigure, and return the {@link
* AutoConfiguredOpenTelemetrySdk}.
*
* @return the {@link AutoConfiguredOpenTelemetrySdk}, or null if {@link #JAVAAGENT_NOOP_CONFIG}
* is enabled
* @return the {@link AutoConfiguredOpenTelemetrySdk}
*/
@Nullable
public static synchronized AutoConfiguredOpenTelemetrySdk installOpenTelemetrySdk(Config config) {
if (config.getBoolean(JAVAAGENT_NOOP_CONFIG, false)) {
static synchronized AutoConfiguredOpenTelemetrySdk installOpenTelemetrySdk(
boolean enableNoopApi, Config config) {
if (enableNoopApi) {
logger.info("Tracing and metrics are disabled because noop is enabled.");
GlobalOpenTelemetry.set(NoopOpenTelemetry.getInstance());
return null;
Expand Down
18 changes: 2 additions & 16 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 @@ -21,31 +21,17 @@ class OpenTelemetryInstallerTest extends Specification {
}

def "should initialize noop"() {
given:
def config = Config.builder()
.readProperties([
(OpenTelemetryInstaller.JAVAAGENT_NOOP_CONFIG) : "true"
])
.build()

when:
def otelInstaller = OpenTelemetryInstaller.installOpenTelemetrySdk(config)
def otelInstaller = OpenTelemetryInstaller.installOpenTelemetrySdk(true, Config.builder().build())

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

def "should NOT initialize noop"() {
given:
def config = Config.builder()
.readProperties([
(OpenTelemetryInstaller.JAVAAGENT_NOOP_CONFIG) : "false",
])
.build()

when:
def otelInstaller = OpenTelemetryInstaller.installOpenTelemetrySdk(config)
def otelInstaller = OpenTelemetryInstaller.installOpenTelemetrySdk(false, Config.builder().build())

then:
otelInstaller != null
Expand Down