Expose AutoConfiguredOpenTelemetrySdk to AgentListener #4831
Conversation
jack-berg
requested review from
anuraaga,
iNikem,
jkwatson,
laurit,
mateuszrzeszutek,
pavolloffay,
trask and
tylerbenson
as code owners
| * <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) {} |
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
it would be really nice to deprecate this. not sure what to do about noop. @jkwatson @breedx-splk are you finding noop useful?
There was a problem hiding this comment.
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.
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
There was a problem hiding this comment.
What about a noop implementation equivalent of the AutoConfiguredOpenTelemetrySdk?
There was a problem hiding this comment.
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
OpenTelemetrySdkinstead ofOpenTelemetry, which gives access toSdkTracerProviderandSdkMeterProviderinstead of justTracerProviderandMeterProvider. This also means that we could give access toSdkLogEmitterProviderwhich will probably never be accessible viaOpenTelemetry. - 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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I prefer having access to
AutoConfiguredOpenTelemetrySdkversusOpenTelemetryandResourcebecause:
- We have access to
OpenTelemetrySdkinstead ofOpenTelemetry, which gives access toSdkTracerProviderandSdkMeterProviderinstead of justTracerProviderandMeterProvider. This also means that we could give access toSdkLogEmitterProviderwhich will probably never be accessible viaOpenTelemetry.- 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.
There was a problem hiding this comment.
@trask if noop OpenTelemetry is installed then the runtime metrics won't have any affect, right?
| 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.
This constant was a duplicate of of AgentInstaller#JAVAAGENT_ENABLED_CONFIG.
| @@ -110,7 +112,9 @@ public static ResettableClassFileTransformer installBytebuddyAgent( | |||
|
|
|||
| setBootstrapPackages(config); | |||
|
|
|||
| runBeforeAgentListeners(agentListeners, config); | |||
| AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = installOpenTelemetrySdk(config); | |||
There was a problem hiding this comment.
installOpenTelemetrySdk(Config) is @Nullable because config may dictate a noop OpenTelemetry. But how is a noop OpenTelemetry different than just deactivating the agent altogether via otel.javaagent.enabled=false?
There was a problem hiding this comment.
see #3053 for history / purpose of noop
| if (autoConfiguredSdk != null) { | ||
| agentListener.afterAgent(autoConfiguredSdk); | ||
| } | ||
| agentListener.afterAgent(config, autoConfiguredSdk); |
There was a problem hiding this comment.
The afterAgent() listener method is also executed from the DelayedAfterAgentCallback (under some specific conditions), can you pass the autoConfiguredSdk there too?
…y#4831) * Expose AutoConfiguredOpenTelemetrySdk to AgentListener * Only call AgentListener if noop is disabled, deprecate AgentListener methods * Call AgentListener in DelayedAfterAgentCallback
Resolves #4791