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 @@ -5,6 +5,8 @@

package io.opentelemetry.api.incubator.config;

import static io.opentelemetry.api.incubator.config.DeclarativeConfigProperties.empty;

import javax.annotation.Nullable;
import javax.annotation.concurrent.ThreadSafe;

Expand All @@ -30,6 +32,31 @@ public interface ConfigProvider {
@Nullable
DeclarativeConfigProperties getInstrumentationConfig();

/**
* Returns the {@link DeclarativeConfigProperties} for a specific instrumentation by name. If no
* configuration is available for the given name, an empty {@link DeclarativeConfigProperties} is
* returned.
*
* @param name the name of the instrumentation
* @return the {@link DeclarativeConfigProperties} for the given instrumentation name
*/
default DeclarativeConfigProperties getInstrumentationConfig(String name) {
DeclarativeConfigProperties config = getInstrumentationConfig();
return config == null ? empty() : config.get("java").get(name);
}

/**
* Returns the {@link DeclarativeConfigProperties} for general instrumentation configuration. If
* the general configuration is not available, an empty {@link DeclarativeConfigProperties} is
* returned.
*
* @return the {@link DeclarativeConfigProperties} for the general instrumentation configuration
*/
default DeclarativeConfigProperties getGeneralInstrumentationConfig() {
Copy link
Member

Choose a reason for hiding this comment

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

would we be comfortable with something shorter, e.g.

  • getInstrumentationGeneral()
  • getInstrumentationJava(name)

Copy link
Member Author

Choose a reason for hiding this comment

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

What about "getGeneralInstrumentation"?

Copy link
Member

Choose a reason for hiding this comment

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

What about "getGeneralInstrumentation"?

I like this for general, but symmetric naming yields getJavaInstrumentation(name), which makes it sound like its accessing the instrumentation rather than the config for the instrumentation.

Not that getInstrumentationJava(name) is especially obvious either, but at least the method name reflects the path to the YAML node being accessed.

Copy link
Member

Choose a reason for hiding this comment

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

maybe

  • getInstrumentation(name)
  • getGeneralInstrumentation()

with the idea that the "Java" is redundant for the Java SDK

Copy link
Member

Choose a reason for hiding this comment

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

Is getInstrumentation(name) too close to getInstrumentationConfig() given the difference in function?

Copy link
Member

Choose a reason for hiding this comment

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

is there a way we could remove getInstrumentationConfig()?

what if we added something like .exists() to DeclarativeConfigurationProperties for the few people who may care about the difference between intermediate nodes being present or not

Copy link
Member

Choose a reason for hiding this comment

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

I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not particularly attached to it, but there's the bit about it being part of the spec. Of course the spec can be changed. But the path of least resistance for the short term is to find method names that jive with the current spec and which can be characterized as syntactic sugar. The spec doesn't prohibit syntactic sugar so it's the sweet spot where we can improve usability quickly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went for

  • getInstrumentationConfig
  • getGeneralInstrumentationConfig

now based on the feedback. Let me know what you think.

DeclarativeConfigProperties config = getInstrumentationConfig();
return config == null ? empty() : config.get("general");
}

/** Returns a no-op {@link ConfigProvider}. */
static ConfigProvider noop() {
return () -> null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,12 @@ void noopEquality() {
ConfigProvider noop = ConfigProvider.noop();
assertThat(ConfigProvider.noop()).isSameAs(noop);
}

@Test
void instrumentationConfigFallback() {
ConfigProvider configProvider = ConfigProvider.noop();
assertThat(configProvider.getInstrumentationConfig()).isNull();
assertThat(configProvider.getInstrumentationConfig("servlet")).isNotNull();
assertThat(configProvider.getGeneralInstrumentationConfig()).isNotNull();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -231,5 +231,16 @@ void configFile_ConfigProvider() {
assertThat(InstrumentationConfigUtil.javaInstrumentationConfig(globalConfigProvider, "example"))
.isNotNull()
.satisfies(exampleConfig -> assertThat(exampleConfig.getString("key")).isEqualTo("value"));

// shortcuts to get specific instrumentation config
assertThat(globalConfigProvider.getInstrumentationConfig("example").getString("key"))
.isEqualTo("value");
assertThat(
globalConfigProvider
.getGeneralInstrumentationConfig()
.get("http")
.get("client")
.getScalarList("request_captured_headers", String.class))
.isEqualTo(Arrays.asList("Content-Type", "Accept"));
}
}
Loading