Spec update for callbacks on ConfigProvider to support runtime changes

[jackshirazi]

Fixes #4899

Changes

This PR extends specification/configuration/api.md to define a language-neutral
ConfigProvider change-listener contract for runtime declarative configuration updates.

Spec updates include:

• adding Add config change listener as a required ConfigProvider operation
• defining watched-path requirements (absolute declarative path, exact-match semantics)
• defining callback payload semantics (path + updated ConfigProperties)
• clarifying empty/unset behavior (newConfig is a valid instance representing an empty mapping node when unset/cleared)
• defining delivery semantics (coalescing allowed, ordering unspecified)
• defining lifecycle/concurrency behavior (idempotent close, post-close behavior, concurrency expectations)
• defining error/unsupported-provider behavior (listener failure isolation, no-op registration when notifications are unsupported)

• Related issues #, Making "methods" instrumentation dynamically updateable opentelemetry-java-instrumentation#15228
• Related OTEP(s) [OTEP] Telemetry Policy #4738
• Links to the prototypes (when adding or changing features): Add a ConfigProvider callback for runtime instrumentation option changes opentelemetry-java#8076
• CHANGELOG.md file updated for non-trivial changes
• Spec compliance matrix updated if necessary

[trask]

these could probably be standard across languages

worth noting whether traversing through arrays is supported

[jackshirazi]

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!

[trask]

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?

[jackshirazi]

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

[trask]

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

[jack-berg]

I think it would be good to standardize on something like JSONPath:

• https://en.wikipedia.org/wiki/JSONPath
• https://www.rfc-editor.org/rfc/rfc9535

Or maybe some sort of abbreviated / subset of the syntax which achieves the goal while keeping the implementation burden reasonable.

[jackshirazi]

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

[jack-berg]

Or are we proposing to specify it here?

This. There hasn't been a need to specify path syntax in declarative config so far.

[jack-berg]

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.

[jackshirazi]

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?

[jack-berg]

We could just have newConfig exactly mirror the the underlying data model:

• If a watched node is unset or cleared, call onChange with newConfig = null.
• If a watched node is set to empty, call onChange with newConfig = {}

[jack-berg]

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.

[jackshirazi]

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

[jack-berg]

This doc is the declarative config API doc?

[jack-berg]

As declarative config integrates more tightly into the otel java agent, and as we start looking towards dynamic config solutions like #4738, I think a capability to allow instrumentation to respond to changes in config is essential.

Based on #4889, only PHP and Java have implemented the ConfigProvider API. So curious if @Nevay / @brettmc have identified any need for this.

As for other declarative config implementers, @codeboten, @MikeGoldsmith, @maryliag, @Kielek, @ysolomchenko, @marcalff, even if you haven't implemented ConfigProvider API yet, does this use case listening for config changes resonate with you?

[MikeGoldsmith]

A way to watch a config and automatically reload would be welcome to remove the need to restart a service to pick up new changes. I don't think it would be a hard requirement though.

[Kielek]

@jack-berg, the hot reload functionality sounds great, but it should not be marked as required functionality. It should be up to the technology to decide if it can be implemented or no.

I suppose that also partial support can be considered with returned information to configuration provide (OpAMP?) that some settings cannot be applied without process restart.

[pellared]

Is there any prototype for this?

[pellared]

@jack-berg, the hot reload functionality sounds great, but it should not be marked as required functionality. It should be up to the technology to decide if it can be implemented or no.

I suppose that also partial support can be considered with returned information to configuration provide (OpAMP?) that some settings cannot be applied without process restart.

What is more, making (especially everything) "hot reload" will make the SDK less efficient because of required additional synchronization.

In my opinion, the prototype should include extensive benchmarks. I am worried that this is going to add more synchronization on the hot path.

[jack-berg]

What is more, making (especially everything) "hot reload" will make the SDK less efficient because of required additional synchronization.

The hot reload proposed here is limited only to ConfigProvider, the API portion of declarative config which instrumentations use for configuration. So its not on the hot path of the internals of the SDK, but the synchornization would still on the hot path for each individual instrumentation. I.e. if an http instrumentation supports dynamic config, it would have to synchronize the logic that determines if / which HTTP request / response headers to capture (amongst other things).

This convo reminds me of the convo #4645. I initially pushed back, favoring eventual visibility without guarantees for performance reasons, but was ultimately convinced that an additional .8ns per record operation was low enough overhead to not worry. I believe the same level of synchronization and overhead would occur here as a result of instrumentation config changing.

Is there any prototype for this?

@jackshirazi has been sketching out the API here. Notably, there is no SDK implementation, nor proposed SDK spec here. I think that needs to change.

@jack-berg, the hot reload functionality sounds great, but it should not be marked as required functionality.

Yeah we should talk about this. Besides the potential performance overhead from runtime changes to instrumentation config, there's also the additional complexity required. Even if every language supported the ability to watch for changes, we can't force every instrumentation to call those watch APIs (although we could encourage, similar to how we don't force semantic conventions but encourage). What does it mean for the UX if only some instrumentation is written to be responsive to runtime changes?

[jackshirazi]

Runtime changes are for few and select components. The TelemetryPolicy that this aligns to does not at all expect reload of all components, nor even that they be enabled to do so. The intention is that IF some component is enabled to handle runtime changes, THEN there is a mechanism for it to receive those changes. For Java, there are maybe a dozen components that will be implemented to adapt to runtime configuration changes, and at the moment only one instrumentation that is proposed to adapt to runtime changes. This is very targeted.

• Only the components that are interested in runtime config changes will add a callback for the path that they are interested in, this is always likely to be a small set
• The TelemetryPolicy pipeline that handles runtime config changes will only accept changes that are configured to be implemented
• For an SDK, these are expected to be rare events (you only occasionally reconfigure the agent, eg for the most common example of changing sampling rate, you might change it at most a few times over the day)
• The nature of config changes are that they are not expected to be applied instantaneously, especially since the main impetus is for a remote central config to provide changes. An eventually consistent approach is fine
• The biggest overhead that I can see is where there is a mismatch at the level between the path that is being registered for a callback and the path that is used to make a config change. If we specify the path to be a standardized string path with dot separators per the example, it becomes a substring match which eliminates that overhead

[github-actions]

This PR was marked stale. It will be closed in 14 days without additional activity.

[jack-berg]

@pellared - thoughts on @jackshirazi's response?

@jackshirazi please respond to the other active threads if you plan on continuing working on this. Thanks!

[jackshirazi]

I can get back to this next week

[reyang]

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"?

[jackshirazi]

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)

[reyang]

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.

[jack-berg]

Is there some sort of difficulty / throughput problem to ensure that calls to update config result in invocation of the callback in the same order? In the prototype we do synchronize the important bits of the setConfig implementation: https://github.com/open-telemetry/opentelemetry-java/pull/8076/changes#diff-5bbe61601fc8c8b7fe1dfbca259e3b6246919fa8e60ed23100c385182e4a4812R118-R127

[reyang]

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

[jackshirazi]

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

[reyang]

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.

[jackshirazi]

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

[reyang]

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?

[jackshirazi]

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

[jack-berg]

FYI, @jackshirazi feel free to resolve comment threads if you think they've reached a conclusion. If the commenter disagrees, they can always unresolve the comment.

[reyang]

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

[jackshirazi]

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

[reyang]

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.

[jackshirazi]

Notably, there is no SDK implementation, nor proposed SDK spec here. I think that needs to change.

@jack-berg I can start working on an SDK implementation, I wasn't sure we had reached that stage. I'll work against the proposed API

[github-actions]

This PR was marked stale. It will be closed in 14 days without additional activity.

[github-actions]

This PR was marked stale. It will be closed in 14 days without additional activity.

[jackshirazi]

Progress is currently focused on the POC which may have feedback into this spec once it's settled

[github-actions]

This PR was marked stale. It will be closed in 14 days without additional activity.

[github-actions]

Closed as inactive. Feel free to reopen if this PR is still being worked on.

[jackshirazi]

please reopen this, I don't seem to have permission to do so. Progress is currently focused on the POC which may have feedback into this spec once it's settled

[reyang]

please reopen this, I don't seem to have permission to do so. Progress is currently focused on the POC which may have feedback into this spec once it's settled

@jackshirazi I've reopened it per your request.

[jack-berg]

I think we should move this to the SDK implementation of ConfigProvider: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/configuration/sdk.md#configprovider