-
Notifications
You must be signed in to change notification settings - Fork 829
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.
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
Update autoconfigure to append signal path to otlp http endpoint if n… #3666
Changes from 4 commits
b3a1b7e
0793d5f
58c8d2a
ce32b38
1557491
3887ef4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,11 +5,20 @@ | |
|
||
package io.opentelemetry.sdk.autoconfigure; | ||
|
||
import static io.opentelemetry.sdk.autoconfigure.OtlpConfigUtil.DATA_TYPE_METRICS; | ||
import static io.opentelemetry.sdk.autoconfigure.OtlpConfigUtil.DATA_TYPE_TRACES; | ||
import static io.opentelemetry.sdk.autoconfigure.OtlpConfigUtil.PROTOCOL_GRPC; | ||
import static io.opentelemetry.sdk.autoconfigure.OtlpConfigUtil.PROTOCOL_HTTP_PROTOBUF; | ||
import static org.assertj.core.api.Assertions.assertThat; | ||
import static org.assertj.core.api.Assertions.assertThatCode; | ||
import static org.assertj.core.api.Assertions.assertThatThrownBy; | ||
|
||
import com.google.common.collect.ImmutableMap; | ||
import io.opentelemetry.sdk.autoconfigure.spi.ConfigurationException; | ||
import java.util.Collections; | ||
import java.util.Map; | ||
import java.util.concurrent.atomic.AtomicReference; | ||
import org.assertj.core.api.ThrowableAssert.ThrowingCallable; | ||
import org.junit.jupiter.api.Test; | ||
|
||
class OtlpConfigUtilTest { | ||
|
@@ -19,7 +28,7 @@ void getOtlpProtocolDefault() { | |
assertThat( | ||
OtlpConfigUtil.getOtlpProtocol( | ||
DATA_TYPE_TRACES, DefaultConfigProperties.createForTest(Collections.emptyMap()))) | ||
.isEqualTo("grpc"); | ||
.isEqualTo(PROTOCOL_GRPC); | ||
|
||
assertThat( | ||
OtlpConfigUtil.getOtlpProtocol( | ||
|
@@ -58,4 +67,121 @@ void getOtlpProtocolDefault() { | |
"otel.exporter.otlp.traces.protocol", "qux")))) | ||
.isEqualTo("qux"); | ||
} | ||
|
||
@Test | ||
void configureOtlpExporterBuilder_ValidEndpoints() { | ||
assertThatCode( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "http://localhost:4317"))) | ||
.doesNotThrowAnyException(); | ||
assertThatCode( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "http://localhost:4317/"))) | ||
.doesNotThrowAnyException(); | ||
assertThatCode( | ||
configureEndpoint(ImmutableMap.of("otel.exporter.otlp.endpoint", "http://localhost"))) | ||
.doesNotThrowAnyException(); | ||
assertThatCode( | ||
configureEndpoint(ImmutableMap.of("otel.exporter.otlp.endpoint", "https://localhost"))) | ||
.doesNotThrowAnyException(); | ||
assertThatCode( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "http://foo:bar@localhost"))) | ||
.doesNotThrowAnyException(); | ||
assertThatCode( | ||
configureEndpoint( | ||
ImmutableMap.of( | ||
"otel.exporter.otlp.endpoint", "http://localhost:4317/path", | ||
"otel.exporter.otlp.protocol", "http/protobuf"))) | ||
.doesNotThrowAnyException(); | ||
} | ||
|
||
@Test | ||
void configureOtlpExporterBuilder_InvalidEndpoints() { | ||
assertThatThrownBy( | ||
configureEndpoint(ImmutableMap.of("otel.exporter.otlp.endpoint", "/foo/bar"))) | ||
.isInstanceOf(ConfigurationException.class) | ||
.hasMessageContaining("OTLP endpoint must be a valid URL:"); | ||
assertThatThrownBy( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "file://localhost:4317"))) | ||
.isInstanceOf(ConfigurationException.class) | ||
.hasMessageContaining("OTLP endpoint scheme must be http or https:"); | ||
assertThatThrownBy( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "http://localhost:4317?foo=bar"))) | ||
.isInstanceOf(ConfigurationException.class) | ||
.hasMessageContaining("OTLP endpoint must not have a query string:"); | ||
assertThatThrownBy( | ||
configureEndpoint( | ||
ImmutableMap.of("otel.exporter.otlp.endpoint", "http://localhost:4317#fragment"))) | ||
.isInstanceOf(ConfigurationException.class) | ||
.hasMessageContaining("OTLP endpoint must not have a fragment:"); | ||
assertThatThrownBy( | ||
configureEndpoint( | ||
ImmutableMap.of( | ||
"otel.exporter.otlp.endpoint", "http://localhost:4317/path", | ||
"otel.exporter.otlp.protocol", "grpc"))) | ||
.isInstanceOf(ConfigurationException.class) | ||
.hasMessageContaining("OTLP endpoint must not have a path:"); | ||
} | ||
|
||
private static ThrowingCallable configureEndpoint(Map<String, String> properties) { | ||
return () -> | ||
OtlpConfigUtil.configureOtlpExporterBuilder( | ||
DATA_TYPE_TRACES, | ||
DefaultConfigProperties.createForTest(properties), | ||
value -> {}, | ||
(value1, value2) -> {}, | ||
value -> {}, | ||
value -> {}, | ||
value -> {}); | ||
} | ||
|
||
@Test | ||
void configureOtlpExporterBuilder_HttpProtobufEndpoint() { | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317")) | ||
.isEqualTo("http://localhost:4317/v1/traces"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/")) | ||
.isEqualTo("http://localhost:4317/v1/traces"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add a test for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that would violate the spec. Configuring a per-signal endpoint should not append anything, so |
||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/foo")) | ||
.isEqualTo("http://localhost:4317/foo/v1/traces"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/foo/")) | ||
.isEqualTo("http://localhost:4317/foo/v1/traces"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces")) | ||
.isEqualTo("http://localhost:4317/v1/traces"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/metrics")) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks - this is a descriptive and good case to have here |
||
.isEqualTo("http://localhost:4317/v1/metrics/v1/traces"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this one makes sense to me (I mean...it's almost certainly wrong, but it's what the user requested). However, the one where we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is exactly what the spec is forbidding. Better the users notice the error earlier than much later when they have deployed that config everywhere and want to start using metrics and it ends up at There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I misunderstood and you meant the signal-specific thing? That seems like a bug indeed. I'll comment above. #3666 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I was suggesting that if the user provides a trace endpoint as the "generic" endpoint, that we don't mess with it for traces, since it's likely(?) that they are only using tracing, and don't care about metrics and what endpoint might have been populated for it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's currently against the spec and I think delaying the error until the point in time where the user uses any other signal is not good. It would be better to ensure that the (404?) error that occurs with the doubled signal path is easy to find (e.g. by logging). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. I understand that side of the argument. This is potentially a breaking change for some users, so we should be very sure to call it out very clearly in our release notes/CHANGELOG. @jack-berg Can you craft a CHANGELOG entry for this PR? Thanks! |
||
|
||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317")) | ||
.isEqualTo("http://localhost:4317/v1/metrics"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317/")) | ||
.isEqualTo("http://localhost:4317/v1/metrics"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317/foo")) | ||
.isEqualTo("http://localhost:4317/foo/v1/metrics"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317/foo/")) | ||
.isEqualTo("http://localhost:4317/foo/v1/metrics"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317/v1/metrics")) | ||
.isEqualTo("http://localhost:4317/v1/metrics"); | ||
assertThat(configureHttpProtobufEndpoint(DATA_TYPE_METRICS, "http://localhost:4317/v1/traces")) | ||
.isEqualTo("http://localhost:4317/v1/traces/v1/metrics"); | ||
} | ||
|
||
private static String configureHttpProtobufEndpoint(String dataType, String configuredEndpoint) { | ||
AtomicReference<String> endpoint = new AtomicReference<>(""); | ||
|
||
OtlpConfigUtil.configureOtlpExporterBuilder( | ||
dataType, | ||
DefaultConfigProperties.createForTest( | ||
ImmutableMap.of( | ||
"otel.exporter.otlp.protocol", PROTOCOL_HTTP_PROTOBUF, | ||
"otel.exporter.otlp.endpoint", configuredEndpoint)), | ||
endpoint::set, | ||
(value1, value2) -> {}, | ||
value -> {}, | ||
value -> {}, | ||
value -> {}); | ||
|
||
return endpoint.get(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a little liberty from the spec here by appending a
/
before the signal path if it doesn't exist.A strict interpretation would cause
http://localhost:4317/foo
to be transformed tohttp://localhost:4317/foov1/traces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's OK. If someone needs a path without slash (or something entirely different), they can configure it with the per-signal configuration.
Still, it should probably also be fixed in the spec.