Skip to content
Merged
Show file tree
Hide file tree
Changes from 11 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 @@ -7,11 +7,35 @@

import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.incubator.config.ConfigProvider;
import io.opentelemetry.api.incubator.config.DeclarativeConfigProperties;

/** Extension to {@link OpenTelemetry} with experimental APIs. */
public interface ExtendedOpenTelemetry extends OpenTelemetry {
/** Returns the {@link ConfigProvider} for this {@link OpenTelemetry}. */
default ConfigProvider getConfigProvider() {
return ConfigProvider.noop();
}

/**
* 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.
*
Comment thread
zeitlinger marked this conversation as resolved.
* @param name the name of the instrumentation
* @return the {@link DeclarativeConfigProperties} for the given instrumentation name
*/
default DeclarativeConfigProperties getInstrumentationConfig(String name) {
Comment thread
trask marked this conversation as resolved.
return getConfigProvider().getInstrumentationConfig(name);
}

/**
* Returns the {@link DeclarativeConfigProperties} for general instrumentation configuration. If
* the general configuration is not available, an empty {@link DeclarativeConfigProperties} is
Comment thread
zeitlinger marked this conversation as resolved.
Outdated
* returned.
*
* @return the {@link DeclarativeConfigProperties} for the general instrumentation configuration
*/
default DeclarativeConfigProperties getGeneralInstrumentationConfig() {
return getConfigProvider().getGeneralInstrumentationConfig();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,29 @@ public interface ConfigProvider {
*/
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) {
return getInstrumentationConfig().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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

return getInstrumentationConfig().get("general");
}

/** Returns a no-op {@link ConfigProvider}. */
static ConfigProvider noop() {
return DeclarativeConfigProperties::empty;
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()).isNotNull();
assertThat(configProvider.getInstrumentationConfig("servlet")).isNotNull();
assertThat(configProvider.getGeneralInstrumentationConfig()).isNotNull();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
package io.opentelemetry.api.incubator;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.when;

import io.opentelemetry.api.GlobalOpenTelemetry;
import io.opentelemetry.api.OpenTelemetry;
import io.opentelemetry.api.incubator.config.ConfigProvider;
import io.opentelemetry.api.incubator.logs.ExtendedDefaultLoggerProvider;
import io.opentelemetry.api.incubator.logs.ExtendedLogger;
import io.opentelemetry.api.incubator.metrics.ExtendedDefaultMeterProvider;
Expand All @@ -22,8 +25,12 @@
import io.opentelemetry.context.propagation.ContextPropagators;
import io.opentelemetry.sdk.OpenTelemetrySdk;
import io.opentelemetry.sdk.extension.incubator.ExtendedOpenTelemetrySdk;
import io.opentelemetry.sdk.extension.incubator.fileconfig.DeclarativeConfiguration;
import io.opentelemetry.sdk.extension.incubator.fileconfig.SdkConfigProvider;
import io.opentelemetry.sdk.extension.incubator.fileconfig.internal.model.OpenTelemetryConfigurationModel;
import java.io.ByteArrayInputStream;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -70,4 +77,50 @@ private static void assertIsExtended(OpenTelemetry openTelemetry) {
assertThat(openTelemetry.getLogsBridge().get("test")).isInstanceOf(ExtendedLogger.class);
assertThat(openTelemetry.getTracer("test")).isInstanceOf(ExtendedTracer.class);
}

@Test
void instrumentationConfig() {
String configYaml =
"instrumentation/development:\n"
+ " general:\n"
+ " http:\n"
+ " client:\n"
+ " request_captured_headers:\n"
+ " - client-request-header1\n"
+ " - client-request-header2\n"
+ " java:\n"
+ " example:\n"
+ " property: \"value\"";

OpenTelemetryConfigurationModel configuration =
DeclarativeConfiguration.parse(
new ByteArrayInputStream(configYaml.getBytes(StandardCharsets.UTF_8)));
SdkConfigProvider configProvider = SdkConfigProvider.create(configuration);
ExtendedOpenTelemetry openTelemetry =
ExtendedOpenTelemetrySdk.create(OpenTelemetrySdk.builder().build(), configProvider);

// shortcuts to get specific instrumentation config
assertThat(openTelemetry.getInstrumentationConfig("example").getString("property"))
.isEqualTo("value");
assertThat(
openTelemetry
.getGeneralInstrumentationConfig()
.get("http")
.get("client")
.getScalarList("request_captured_headers", String.class))
.isEqualTo(Arrays.asList("client-request-header1", "client-request-header2"));
}

@Test
void instrumentationConfigFallback() {
ConfigProvider configProvider = ConfigProvider.noop();
// cannot create ExtendedOpenTelemetry with noop ConfigProvider right now,
// but will be possible once stable API is available
ExtendedOpenTelemetry openTelemetry = spy(ExtendedOpenTelemetry.class);
when(openTelemetry.getConfigProvider()).thenReturn(configProvider);

assertThat(configProvider.getInstrumentationConfig()).isNotNull();
assertThat(openTelemetry.getInstrumentationConfig("servlet")).isNotNull();
assertThat(openTelemetry.getGeneralInstrumentationConfig()).isNotNull();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -224,5 +224,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");
Comment thread
zeitlinger marked this conversation as resolved.
assertThat(
globalConfigProvider
.getGeneralInstrumentationConfig()
.get("http")
.get("client")
.getScalarList("request_captured_headers", String.class))
.isEqualTo(Arrays.asList("Content-Type", "Accept"));
}
}