-
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
Conversation
* <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.
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()
.
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 finding noop
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.
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.
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 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 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
. - 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.
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?
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 prefer having access to
AutoConfiguredOpenTelemetrySdk
versusOpenTelemetry
andResource
because:
- We have access to
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
.- 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.
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?
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 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
.
@@ -110,7 +112,9 @@ public static ResettableClassFileTransformer installBytebuddyAgent( | |||
|
|||
setBootstrapPackages(config); | |||
|
|||
runBeforeAgentListeners(agentListeners, config); | |||
AutoConfiguredOpenTelemetrySdk autoConfiguredSdk = installOpenTelemetrySdk(config); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #3053 for history / purpose of noop
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.
thx!
if (autoConfiguredSdk != null) { | ||
agentListener.afterAgent(autoConfiguredSdk); | ||
} | ||
agentListener.afterAgent(config, 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.
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