add dynamically disabled instrumentation capability #422
Conversation
| try { | ||
| InstrumentationScopeInfo instrumentationScopeInfo = | ||
| getInstrumentationScopeInfo(sdkTracer); | ||
| TracerConfig tConfig = getTracerConfig(finalProvider, instrumentationScopeInfo); |
There was a problem hiding this comment.
Given the tools we have available upstream, I can see why we need to do the config update this way, though I was wondering (not sure if it has been discussed) if instead of evaluating a field here, we could instead change that line by something like if (!tracerConfig.isEnabled()) { instead? That way we might not need to keep a background thread alive constantly checking for config changes and applying them one by one by getting a new config and then resetting the tracerEnabled field as we could do the check reactively whenever spanBuilder is called.
I guess a downside might be that, depending on what's done in the TracerConfig.isEnabled() impl, it might cause some delays, though a good implementation shouldn't take long to return.
There was a problem hiding this comment.
If the delay concerns are a blocker, I think another alternative might be to make TracerConfig observable and then make the SdkTracer impl subscribe to changes in it, updating its tracerEnabled field whenever TracerConfig notifies about a change.
There was a problem hiding this comment.
delay concerns are a blocker
They are not a blocker. It does not need to be immediate, it's fine for the instrumentation to be disabled but that not take effect for a while. Also, adding an observer-subscription model doesn't change the delay, because the delay is from SdkTracer.tracerEnabled being non-volatile. callbacks wouldn't change anything because the state change needs to propagate across threads and only making it volatile would speed that. I originally had it volatile in the PR that made it non-final, but the maintainers decided to go with a non-volatile implementation, at least for now
There was a problem hiding this comment.
background thread alive constantly checking for config changes
The background thread checker is no on by default. I expect normal operation for something else - eg an OpAMP client - to do regular checking from central config (and maybe property and config file, to be decided) and update the instrumentation enablement from that. The background thread checker is there mainly so that testing can be done cleanly, though it is fully operational if someone wanted to use it in production
There was a problem hiding this comment.
They are not a blocker. It does not need to be immediate
Sorry I think I didn't properly explain what I meant by delay, I was more talking about the possibility that by changing this condition to always query the config by calling TracerConfig.isEnabled() instead of evaluating a field, then it could cause a performance overhead to the instrumented app in case that the provided TracerConfig impl was poorly created, for example, let's say a TracerConfig impl reads a file every time its isEnabled method is called, then that could block the span creation process for a little while, causing the host app performance to get affected every time it creates a span. On the other hand, if we decide to keep the field then I agree we should make it volatile, though I was more of the opinion of removing that field and instead evaluating the config on every span creation.
There was a problem hiding this comment.
Though it makes sense what you mentioned about the OpAMP client triggering the update process when it gets a new config. I guess with the alternatives I mentioned I was also trying to make the "update process" something that couldn't be triggered from the outside since I'm not sure if it'd be desirable for any part of the code that might have access to the Otel instance (including via the GlobalOpenTelemetry singleton) to make config changes. However, if it's only a matter of adding the updateTracerConfigurations() method without params, but only as a sort of "notify changes" signal to get the tracer to re-evaluate its existing config provider, then it should be fine.
There was a problem hiding this comment.
in case that the provided
TracerConfigimpl was poorly created,
I tried to open the TracerConfig to be implementable by the user, but the maintainers preferred to keep it as is, essentially a boolean. So atm it's not possible for it to be inefficient
custom/src/main/java/co/elastic/otel/config/DynamicInstrumentation.java
Outdated
Show resolved
Hide resolved
custom/src/main/java/co/elastic/otel/config/DynamicInstrumentation.java
Outdated
Show resolved
Hide resolved
…tion.java Co-authored-by: Jonas Kunz <Jonas.Kunz@gmx.net>
The trace scoping in the SDK is still experimental at this point, so the relevant methods are non-public, hence the use of reflection to access them. Also, dynamically changing scope enablement is additionally experimental with the only additional support in SdkTracer.tracerEnabled being mutable (non-final). As it turns out, this is all that is needed to enable dynamically disabling instrumentation by scope, using reflection.