Add a ConfigProvider callback for runtime instrumentation option changes#8076
Add a ConfigProvider callback for runtime instrumentation option changes#8076jackshirazi wants to merge 3 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.19% -0.01%
Complexity 7594 7594
============================================
Files 841 841
Lines 22915 22916 +1
Branches 2290 2290
============================================
Hits 20670 20670
Misses 1529 1529
- Partials 716 717 +1 ☔ 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
| */ | ||
| 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.
| * @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.
| * @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?
| * <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?
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):