[File based config] Implement profiler configuration customizer to support JFR#2519
[File based config] Implement profiler configuration customizer to support JFR#2519robsunday merged 19 commits intosignalfx:mainfrom
Conversation
This reverts commit 480405a.
…guration.java Co-authored-by: jason plumb <75337021+breedx-splk@users.noreply.github.com>
Minor logging improvements
… URL for declarative config
| private static final JFR instance = new JFR(); | ||
| private static final boolean jfrAvailable = checkJfr(); | ||
|
|
||
| public static JFR getInstance() { |
There was a problem hiding this comment.
[for reviewer] Refactored to make it more OO and mockable.
| if (isProfilerEnabled(model)) { | ||
| if (jfrIsAvailable()) { |
| } | ||
|
|
||
| @VisibleForTesting | ||
| OpenTelemetryConfigurationModel customizeModel(OpenTelemetryConfigurationModel model) { |
There was a problem hiding this comment.
do you need to use DeclarativeConfigurationCustomizer for this at all? You aren't changing the configuration model but just reading it. Couldn't you just use something like AgentInstrumentationConfig.get().getBoolean(SplunkConfiguration.PROFILER_ENABLED_PROPERTY) to get the value for both declarative config and system properties based config (at least in upstream instrumentation conf can be read like that)?
There was a problem hiding this comment.
So this is a port of SdkCustomizer that runs when env var based configuration is in use. I had to implement similar solution for declarative configuration.
I agree I don't modify the configuration here, but SdkCustomizer does not do it either.
Some time ago I asked on Java SIG where I could implement some validations and I was advised to do it in DeclarativeConfigurationCustomizer. That's why I assumed it is OK to utilize it here as well.
There is a call to ContextStorage.addWrapper in line 41. Where, in your opinion, I could safely make this call if I do not use DeclarativeConfigurationCustomizer, so the wrapper is attached early enough?
There was a problem hiding this comment.
I guess the SdkCustomizer is written they way it is to get access to config properties. I think you could consider deleting it and using a AgentListener or BeforeAgentListener. There you can get access to config properties using https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/runtime-telemetry-java8/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/runtimemetrics/java8/JarAnalyzerInstaller.java or you could use AgentInstrumentationConfig like in https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/main/instrumentation/runtime-telemetry/runtime-telemetry-java8/javaagent/src/main/java/io/opentelemetry/instrumentation/javaagent/runtimemetrics/java8/Java8RuntimeMetricsInstaller.java I don't know about config properties but AgentInstrumentationConfig should be able to handle both the declarative and sysprop based configuration.
There was a problem hiding this comment.
Getting rid of customizers (for both: declarative config and env var config) worked. It was enough to call ContextStorage.addWrapper from JfrActivator.
Nice suggestion!
| import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.OpenTelemetryConfigurationModel; | ||
| import java.util.Map; | ||
|
|
||
| public class ProfilerDeclarativeConfigUtil { |
There was a problem hiding this comment.
is this class used anywhere?
There was a problem hiding this comment.
Not any longer, removed
2 participants
Customizer is required here.