Add dynamically changing the inferred span interval #2153
Conversation
jackshirazi
changed the title
|
🔧 The result from spotlessApply was committed to the PR branch. |
| public void reschedule() { | ||
| Future<?> future = this.profilingTask; | ||
| if (future != null) { | ||
| if (future.cancel(true)) { |
There was a problem hiding this comment.
| if (future.cancel(true)) { | |
| if (future.isDone() || future.cancel(true)) { |
So the javadoc for Future.cancel() says that it returns
"
falseif the task could not be cancelled, typically because it has already completed;
If the thing finishes right before/when this reschedule() is invoked, we still want to kick it off again, right?
There was a problem hiding this comment.
When the task has already completed, it has already scheduled the next task, so on isDone() we do NOT want to schedule a new task in this reschedule() method
| config.setProfilerInterval(interval); | ||
| profiler.reschedule(); |
There was a problem hiding this comment.
If I get this correctly, when we "disable at runtime", it means:
- a very long profiler interval is configured
reschedule()is executed and schedules profiling execution very far in the future
When we then "enable it at runtime", it means:
- a relatively short profiler interval is configured
reschedule()is executed and schedules profiling execution accordingly to the interval (so in the close future)
In the case where the value set to disable is not large enough, for example 1 day, then it means that we will get one extra scheduled execution, which is probably not what we expect here.
I think it would probably be better to define a specific value/threshold to indicate that the profiler should be considered as disabled (zero or a significant value), it could then allow to avoid re-scheduling when such value is configured.
There was a problem hiding this comment.
This is documentation rather than anything for the code. The code change is not disabling anything, it's purely making the interval dynamically changeable.
A separate note for the doc (I'll do that in another change as I need to think about also how to explain using this dynamic capability for an example) is that choosing a very long time (eg max long) effectively disables the inferred spans, while leaving it in a state where it could be re-enabled
Description:
I'd like to dynamically enable/disable inferred spans, but it looks like a lot of work to get to dynamically initializing and loading/unloading. So instead I've gone for this option, which is to let the interval be dynamically updated. You would need to have inferred spans enabled to use it, but after that you could change the interval, including to a very large value that effectively stops inferred spans doing anything (though the objects would all still be there and loaded)
Existing Issue(s):
None, though part of the overall "making the agent more dynamic" task set eg open-telemetry/opentelemetry-java-instrumentation#12251
Testing:
Test class added, but end-to-end testing still needs adding (it's not straightforward but I'll do that when I test these changes in our distribution). The changes shouldn't affect the existing functionality
Documentation:
It needs additional documentation on an example of how to use this feature, that's to come after teh above testing
Outstanding items:
Currently the changes are completely API based. I'm not proposing to add any config based support, but that could be added easily. End-to-end testing and example documentation of using the API are needed