-
Notifications
You must be signed in to change notification settings - Fork 966
Add a ConfigProvider callback for runtime instrumentation option changes #8076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 3 commits
a21685f
1b4d51b
164ea55
39b9c63
6277dd7
8ed6575
1858add
3617b16
cb51d1f
d243825
1a244dc
c69e75b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,20 @@ default DeclarativeConfigProperties getGeneralInstrumentationConfig() { | |
| return getInstrumentationConfig().get("general"); | ||
| } | ||
|
|
||
| /** | ||
| * Registers an {@link InstrumentationConfigChangeListener} to receive updates when | ||
| * instrumentation configuration changes. | ||
| * | ||
| * <p>The default implementation performs no registration and returns a no-op handle. | ||
| * | ||
| * @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( | ||
| InstrumentationConfigChangeListener listener) { | ||
| return () -> {}; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need for a default implementation. This is still experimental so we can add new methods without worrying about compatibility.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wdyt about to cover also 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I'll add the path and remove previous |
||
|
|
||
| /** Returns a no-op {@link ConfigProvider}. */ | ||
| static ConfigProvider noop() { | ||
| return DeclarativeConfigProperties::empty; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| /* | ||
| * Copyright The OpenTelemetry Authors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package io.opentelemetry.api.incubator.config; | ||
|
|
||
| import javax.annotation.Nullable; | ||
|
|
||
| /** Listener notified when instrumentation configuration changes. */ | ||
| @FunctionalInterface | ||
| public interface InstrumentationConfigChangeListener { | ||
|
|
||
| /** | ||
| * Called when the effective config for one top-level instrumentation node changes (for example | ||
| * {@code methods}, {@code kafka}, or {@code grpc}). | ||
| * | ||
| * <p>Both config arguments are scoped to {@code instrumentationName}. | ||
| * | ||
| * <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 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need to be able to watch
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll clarify in javadoc along with Trask's suggested change which also changes scope |
||
| * @param previousConfig the previous effective configuration, or {@code null} if unavailable | ||
| * @param newConfig the updated effective configuration for {@code instrumentationName} | ||
| */ | ||
| void onChange( | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| String instrumentationName, | ||
| @Nullable DeclarativeConfigProperties previousConfig, | ||
| DeclarativeConfigProperties newConfig); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with switching to a new ListenerRegistration interface that extends AutoCloseable, wdyt?