-
Notifications
You must be signed in to change notification settings - Fork 969
Spec update for callbacks on ConfigProvider to support runtime changes #4900
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 4 commits
a5acd3a
70d429d
d5911b6
6fe3a42
f9665b4
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 |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ weight: 1 | |
| * [ConfigProvider](#configprovider) | ||
| + [ConfigProvider operations](#configprovider-operations) | ||
| - [Get instrumentation config](#get-instrumentation-config) | ||
| - [Add config change listener](#add-config-change-listener) | ||
| * [ConfigProperties](#configproperties) | ||
|
|
||
| <!-- tocstop --> | ||
|
|
@@ -49,6 +50,7 @@ default `ConfigProvider`, and set/register it. | |
| The `ConfigProvider` MUST provide the following functions: | ||
|
|
||
| * [Get instrumentation config](#get-instrumentation-config) | ||
| * [Add config change listener](#add-config-change-listener) | ||
|
|
||
| TODO: decide if additional operations are needed to improve API ergonomics | ||
|
|
||
|
|
@@ -63,6 +65,64 @@ configuration mapping node. | |
| If the `.instrumentation` node is not set, get instrumentation config SHOULD | ||
| return an empty `ConfigProperties` (as if `.instrumentation: {}` was set). | ||
|
|
||
| ##### Add config change listener | ||
|
|
||
| Register a listener to be notified when configuration at a specific watched path | ||
| changes. | ||
|
|
||
| This API MUST accept the following parameters: | ||
|
|
||
| * `path`: declarative configuration path to watch. | ||
| * `listener`: callback invoked on changes. | ||
|
|
||
| **Returns:** A registration handle with a close operation. | ||
|
|
||
| Path requirements: | ||
|
|
||
| * `path` MUST be an absolute declarative configuration path. | ||
| * `path` matching is exact. Wildcards and prefix matching are not supported. | ||
jackshirazi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * In this version, paths are defined only for named properties. Sequence/array indexing is not supported | ||
| * API implementations SHOULD document accepted path syntax in language-specific | ||
| docs and include examples such as `.instrumentation/development.general.http` | ||
| and `.instrumentation/development.java.methods`. | ||
|
Comment on lines
+85
to
+87
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. these could probably be standard across languages worth noting whether traversing through arrays is supported
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 gave a standard and language specific example, I'm fine with different examples. I've added a line (just before this) about arrays, thanks!
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. oh, I meant about
were you thinking that, e.g. java might use
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, okay I see what you meant. I see what you mean the whole thing should be standardized - is it standardized in declarative config across languages? If so then yes, let's specify standard path syntax accordingly
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. @open-telemetry/configuration-approvers what do you think? thanks
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. I think it would be good to standardize on something like JSONPath: Or maybe some sort of abbreviated / subset of the syntax which achieves the goal while keeping the implementation burden reasonable. |
||
|
|
||
| Callback requirements: | ||
|
|
||
| * If a watched path changes, the callback MUST receive: | ||
| * `path`: the changed watched path. | ||
| * `newConfig`: the updated [`ConfigProperties`](#configproperties) for that | ||
| path. | ||
| * `newConfig` MUST be a valid [`ConfigProperties`](#configproperties) instance | ||
| (never null/nil/None). | ||
| * If the watched node is unset or cleared, `newConfig` MUST represent an empty | ||
| mapping node (equivalent to `{}`). | ||
|
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. Set and empty vs. unset turns out to be semantically meaningful in declarative config: I think we need to find some way signal this difference to watchers. |
||
| * Implementations MAY coalesce rapid successive updates for the same watched | ||
| path. If coalescing is performed, callback delivery MUST use the latest | ||
| configuration state. | ||
| * Ordering of callback delivery is not specified, including for updates touching | ||
| multiple watched paths in one configuration transaction. | ||
|
|
||
| Concurrency and lifecycle requirements: | ||
|
|
||
| * Callback implementations SHOULD be reentrant and SHOULD avoid blocking | ||
| operations. | ||
| * Implementations MUST document callback concurrency guarantees. If they do not, | ||
| users MUST assume callbacks may be invoked concurrently. | ||
| * Closing a registration handle MUST unregister the listener. | ||
|
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 indicate that a callback is required to have a close operation before specifying behavior for a close operation. |
||
| * Close MUST be idempotent (subsequent calls have no effect). | ||
| * After close returns, implementations SHOULD stop new callback delivery for that | ||
| registration. A callback already in progress MAY complete. | ||
| * Registrations SHOULD be dropped automatically when the corresponding | ||
| `ConfigProvider` is shut down or otherwise disposed. | ||
|
|
||
| Error handling and unsupported providers: | ||
|
|
||
| * If callback execution throws an exception, implementations SHOULD isolate the | ||
jackshirazi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| failure to that callback and SHOULD continue notifying other callbacks. | ||
| * If a provider does not support change notifications, registration MUST still | ||
|
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. This is defining the "noop" behavior of this operation. Elsewhere in the spec we have extracted dedicated noop documents (e.g. metrics noop). It may be time to do the same for the declarative config API. |
||
| succeed by returning a no-op registration handle, and callbacks MUST NOT be | ||
| invoked. | ||
|
|
||
| ### ConfigProperties | ||
|
|
||
| `ConfigProperties` is a programmatic representation of a configuration mapping | ||
|
|
||
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.
#nit: could change listener be referring to something other than config? If not, consider dropping.