Skip to content

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.

Sign up for GitHub

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

Open
jackshirazi wants to merge 8 commits into
base: main
Choose a base branch
from
+60 −0
Open

Spec update for callbacks on ConfigProvider to support runtime changes #4900

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a5acd3a
spec for callbacks on ConfigProvider to support runtume changes
jackshirazi Feb 23, 2026
70d429d
Update specification/configuration/api.md
jackshirazi Feb 25, 2026
d5911b6
Apply suggestion from @jackshirazi
jackshirazi Feb 25, 2026
6fe3a42
Update specification/configuration/api.md
jackshirazi Feb 26, 2026
f9665b4
Update specification/configuration/api.md
jackshirazi Mar 4, 2026
69a8b63
Merge branch 'main' into config-provider-callback
jackshirazi Mar 25, 2026
5b0d4d7
Apply suggestion from @jackshirazi
jackshirazi Mar 25, 2026
9371403
jack-berg feedback
jackshirazi Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 60 additions & 0 deletions specification/configuration/api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ weight: 1
* [ConfigProvider](#configprovider)
+ [ConfigProvider operations](#configprovider-operations)
- [Get instrumentation config](#get-instrumentation-config)
- [Add change listener](#add-change-listener)
* [ConfigProperties](#configproperties)

<!-- tocstop -->
Expand Down Expand Up @@ -51,6 +52,7 @@ default `ConfigProvider`, and set/register it.
The `ConfigProvider` MUST provide the following functions:

* [Get instrumentation config](#get-instrumentation-config)
* [Add change listener](#add-change-listener)

TODO: decide if additional operations are needed to improve API ergonomics

Expand All @@ -65,6 +67,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 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. The handle MUST provide a close (or language-equivalent) operation that unregisters the listener.

Path requirements:

* `path` MUST be an absolute declarative configuration path.
Copy link
Copy Markdown

@robsunday robsunday Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to explicitly specify if multiple listeners are allowed for the same path.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good point. Added as the first point in the next Callback requirements

* `path` matching is exact. Wildcards and prefix matching are not supported.
Comment thread
jackshirazi marked this conversation as 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 +87 to +89
Copy link
Copy Markdown
Member

@trask trask Feb 25, 2026

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Feb 25, 2026

Choose a reason for hiding this comment

The 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!

Copy link
Copy Markdown
Member

@trask trask Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I meant about

implementations SHOULD document accepted path syntax

were you thinking that, e.g. java might use .instrumentation/development.general.http path syntax, while another might use something else, e.g. instrumentation/development->general->http?

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Feb 26, 2026

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Member

@trask trask Feb 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@open-telemetry/configuration-approvers what do you think? thanks

Copy link
Copy Markdown
Member

@jack-berg jack-berg Feb 27, 2026

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not specified in declarative config? So can't be specified here? Or are we proposing to specify it here?


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 `{}`).
Copy link
Copy Markdown
Member

@jack-berg jack-berg Feb 27, 2026

Choose a reason for hiding this comment

The 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:

# this is valid
tracer_provider:
  - processors:
       simple:
         exporter:
           console:
---
# this is invalid
tracer_provider:
  - processors:
       simple:
         exporter:

I think we need to find some way signal this difference to watchers.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I agree we should preserve declarative-config semantics. Do you think adding a DeclarativeConfigProperties.unset() (or missing()) is a good option, though it adds more API changes? Or add a constant ConfigChangeListener.UNSET which provides the exact situation, leaving the callback to make the check?

* 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
Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand what this means.

If I make a change "foo=a", then I make another change "foo=b" - is it possible that from the callback I will get "foo=b" first, then later I'll get "foo=a"?

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Especially if those changes are concurrent. I would expect changes to generally be occasional events rather than many close together, so mostly this shouldn't matter, but if there are changes made close together, this doesn't insist on ordering (which could be a pain to implement in some langauges)

Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks @jackshirazi!

@jack-berg WDYT?

I can imagine the following options:

  1. We don't guarantee ordering, and there is no way for clients to reliably determine if it is getting the latest configuration or it is using some old/stale version due to race condition.
  2. We don't send a portion of configuration snapshot to the callback, instead, we just notify the listener "there are some changes which you might be interested", then we expect the listener to go and check the configuration.
  3. In addition to the existing arguments that we pass to the listener callback, we also put something like a sequence number. In the original example, "foo=a" would have sequence number = 1, and "foo=" would have sequence number = 2, then the listener can decide to drop the late arrival notification if the sequence number is smaller than what the listener already got.

multiple watched paths in one configuration transaction.

Concurrency and lifecycle requirements:

* Callback implementations SHOULD be reentrant and SHOULD avoid blocking
Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to understand the thinking behind this - would the configuration component create new threads / execution context for reentrant calls?

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or keep them quick. But the point here is to not force anything on the component, this is telling the component to handle multiple calls as best it can to be a "nice" citizen so the callback isn't expected to add additional overhead to try and handle components

Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick doesn't have a direct relationship to threading/concurrency model.
We can make it quick and sequential.

I guess my main question is - why do we want this to be reentrant? Reentrancy is always more difficult, could be slightly more difficult or significantly more difficult. I want to understand what's the gain/loss by having or not having reentrancy.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick kind of does. If it's quick you can use an exclusive block to do the update and not worry that it's causing problems, which makes the concurrency handling simple.

But it's a SHOULD rather than a MUST. It keeps the change listener implementation simpler. There are likely to be few instrumentations or components that will be adapted to handle callbacks, and most that do are likely to be able to make a simple state update that applies when the instrumentation/component is next applied. So with that expectation, the simpler change listener seems reasonable

Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It keeps the change listener implementation simpler.

Sorry I'm confused. I thought it'll be simpler for the listener if we say "callback will only be invoked sequentially, there is no need for the listener to worry about reentrancy or concurrency". Are we on the same page?

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The simplest change listener implementation is to respond directly to a change in the config and send that directly to the callback. This could be on any thread

Something on any thread -> synchronously changes the config on a path -> synchronously checks for any callbacks on that path -> synchronously does the callback -> instrumentation/component callback implementation handles the callback .

So the change listener here doesn't worry about reentrancy or concurrency and is very simple, and it's all happening on the "any thread" thread. The instrumentation/component callback implementation DOES need to worry about reentrancy and concurrency because there can be more than one "Something on different threads" initiating that. This change listener would document it could execute on any thread and could be calling a change implementation concurrently

A "nicer" but more complex change listener implementation would add every change into a queue, and have a dedicated thread process the queue and apply each callback sequentially. That would document that, and in this case instrumentation/component callback implementations can potentially be simpler (assuming they had additional complexity if they were handling concurrency and re-entrancy).

operations.
* Implementations MUST document callback concurrency guarantees. If they do not,
users MUST assume callbacks may be invoked concurrently.
Comment on lines +111 to +112
Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who are the users and how would they assume? (trying to understand if this is actionable or not)

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user is the implementor of the integration that integrates the ConfigProvider to register a listener, ie mostly instrumentation/component authors. So when that instrumentation or component is now adapted to register for callbacks, it understands what to expect. Eg "callbacks are serialized on one thread" would be nice for the instrumentation/component authors making their job easier, otherwise they have to assume concurrent callbacks which is a pain

Copy link
Copy Markdown
Member

@reyang reyang Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think now I understand your intention better, trying to rephrase and confirm my understanding:

  1. SDK authors MUST document the reentrancy expectations (not guarantees) for listener callbacks.
  2. The instrumentation/component authors MUST handle reentrancy properly (do not support at all, partially support, or fully support), based on the expectations set for the SDK which they are targeting. If there is no clear expectation set by the SDK authors, the instrumentation/component authors MUST support reentrant callbacks.

* Closing a registration handle MUST unregister the listener.
Copy link
Copy Markdown
Member

@jack-berg jack-berg Feb 27, 2026

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. I've updated the return comment to specify that, commit 5b0d4d7

* 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 error, implementations SHOULD isolate the
failure to that callback and SHOULD continue notifying other callbacks.
* If a provider does not support change notifications, registration MUST still
Copy link
Copy Markdown
Member

@jack-berg jack-berg Feb 27, 2026

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Author

@jackshirazi jackshirazi Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is a callout for the declarative config API, not for this doc?

succeed by returning a no-op registration handle, and callbacks MUST NOT be
invoked.

### ConfigProperties

**Status**: [Stable](../document-status.md)
Expand Down
Loading