Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Copy link
Member

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.

InstrumentationConfigChangeListener listener) {
return () -> {};
Copy link
Member

Choose a reason for hiding this comment

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

}
Comment on lines +77 to +80
Copy link
Member

Choose a reason for hiding this comment

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

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


/** Returns a no-op {@link ConfigProvider}. */
static ConfigProvider noop() {
return DeclarativeConfigProperties::empty;
Expand Down
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
Copy link
Member

Choose a reason for hiding this comment

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

Need to be able to watch .instrumentation/development.general.* nodes in addition to .instrumentation/development.java.* nodes. What does instrumentationName look like for these?

* @param previousConfig the previous effective configuration, or {@code null} if unavailable
* @param newConfig the updated effective configuration for {@code instrumentationName}
*/
void onChange(
Copy link
Member

Choose a reason for hiding this comment

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

String instrumentationName,
@Nullable DeclarativeConfigProperties previousConfig,
DeclarativeConfigProperties newConfig);
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package io.opentelemetry.api.incubator;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

import io.opentelemetry.api.incubator.config.ConfigProvider;
import org.junit.jupiter.api.Test;
Expand All @@ -24,5 +25,8 @@ void instrumentationConfigFallback() {
assertThat(configProvider.getInstrumentationConfig()).isNotNull();
assertThat(configProvider.getInstrumentationConfig("servlet")).isNotNull();
assertThat(configProvider.getGeneralInstrumentationConfig()).isNotNull();
AutoCloseable listenerRegistration =
configProvider.addInstrumentationConfigChangeListener((name, previous, current) -> {});
assertThatCode(listenerRegistration::close).doesNotThrowAnyException();
}
}
Loading