Add a ConfigProvider callback for runtime instrumentation option changes#8076
Add a ConfigProvider callback for runtime instrumentation option changes#8076jackshirazi wants to merge 4 commits intoopen-telemetry:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8076 +/- ##
============================================
+ Coverage 90.20% 90.21% +0.01%
- Complexity 7594 7608 +14
============================================
Files 841 841
Lines 22915 22924 +9
Branches 2290 2291 +1
============================================
+ Hits 20670 20682 +12
+ Misses 1529 1526 -3
Partials 716 716 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| default AutoCloseable addInstrumentationConfigChangeListener( | ||
| InstrumentationConfigChangeListener listener) { | ||
| return () -> {}; | ||
| } |
There was a problem hiding this comment.
wdyt about
addConfigChangeListener(".instrumentation.xyz", listener)
to cover also .general nodes, e.g. .general.http.server.request_captured_headers
interface ConfigChangeListener {
void onChange();
}
and listeners can use ConfigProvider to get the latest value (imagining shared code path with initial setup, but not sure)
not sure if previous config is needed? if it's just a rare case, instrumentation could be responsible for maintaining their own previous config
There was a problem hiding this comment.
Agreed. I'll add the path and remove previous
| */ | ||
| default AutoCloseable addInstrumentationConfigChangeListener( | ||
| InstrumentationConfigChangeListener listener) { | ||
| return () -> {}; |
There was a problem hiding this comment.
No need for a default implementation. This is still experimental so we can add new methods without worrying about compatibility.
There was a problem hiding this comment.
True. But this default is a safe fallback, I added it for that reason rather than for compatibility. I'm not against removing and implementing it in the implementations, but it's a good default
| * @param listener the listener to notify when instrumentation configuration changes | ||
| * @return an {@link AutoCloseable} handle that can be closed to unregister the listener | ||
| */ | ||
| default AutoCloseable addInstrumentationConfigChangeListener( |
There was a problem hiding this comment.
I have mixed feelings about using AutoCloseable here. IDE's will warn " used without 'try'-with-resources statement", where this isn't a traditional resource that must be to avoid a memory leak or other problems.
There was a problem hiding this comment.
I'm fine with switching to a new ListenerRegistration interface that extends AutoCloseable, wdyt?
| * @param previousConfig the previous effective configuration, or {@code null} if unavailable | ||
| * @param newConfig the updated effective configuration for {@code instrumentationName} | ||
| */ | ||
| void onChange( |
There was a problem hiding this comment.
So with this pattern the listener is responsible for filtering through these change events and only reacting to ones of interest?
What if an instrumentation is interested in both .instrumentation/development.general.http and .instrumentation/development.java.<library_name>? Presumably, a change to instrumentation config which touched both would result in two separate invocations to the listener?
There was a problem hiding this comment.
Yes, the assumption is listeners only get notified on events they register for. For updates that affect multiple top-level nodes, the expectation is one callback per changed node. I’ll make that explicit in javadocs
| * <p>{@code newConfig} is never null. If the node is unset or cleared, {@code newConfig} is | ||
| * {@link DeclarativeConfigProperties#empty()}. | ||
| * | ||
| * @param instrumentationName the top-level instrumentation name that changed |
There was a problem hiding this comment.
Need to be able to watch .instrumentation/development.general.* nodes in addition to .instrumentation/development.java.* nodes. What does instrumentationName look like for these?
There was a problem hiding this comment.
I'll clarify in javadoc along with Trask's suggested change which also changes scope
3 participants
First step in adding support for runtime changes to options, focusing only on API additions.
The assumption is that if an instrumentation is able to handle changes at runtime, it will register a callback against the ConfigProvider instance that the otel SDK is using.
Further steps (to be discussed):