Update autoconfigure to append signal path to otlp http endpoint if n… #3666

jack-berg · 2021-09-27T16:18:42Z

…ot present

Resolves #3650.

…ot present

jack-berg · 2021-09-27T16:24:52Z

...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java

+          && !endpoint.endsWith(signalPath)) {
+        if (!endpoint.endsWith("/")) {
+          endpoint += "/";
+        }


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 to http://localhost:4317/foov1/traces

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.

codecov · 2021-09-27T16:27:13Z

Codecov Report

Merging #3666 (ce32b38) into main (c87e2b3) will increase coverage by 0.70%.
The diff coverage is 91.30%.

❗ Current head ce32b38 differs from pull request most recent head 3887ef4. Consider uploading reports for the commit 3887ef4 to get more accurate results

@@             Coverage Diff              @@
##               main    #3666      +/-   ##
============================================
+ Coverage     88.86%   89.57%   +0.70%     
- Complexity     3689     3784      +95     
============================================
  Files           442      450       +8     
  Lines         11562    11822     +260     
  Branches       1113     1134      +21     
============================================
+ Hits          10275    10589     +314     
+ Misses          906      858      -48     
+ Partials        381      375       -6

Impacted Files	Coverage Δ
...pentelemetry/sdk/autoconfigure/OtlpConfigUtil.java	`92.30% <90.47%> (-2.70%)`	⬇️
...sdk/autoconfigure/MetricExporterConfiguration.java	`77.33% <100.00%> (ø)`
...y/sdk/autoconfigure/SpanExporterConfiguration.java	`94.66% <100.00%> (ø)`
...ce/jaeger/sampler/JaegerRemoteSamplerProvider.java	`95.23% <0.00%> (-4.77%)`	⬇️
...emetry/exporter/otlp/internal/ProtoSerializer.java	`95.89% <0.00%> (-4.11%)`	⬇️
...elemetry/sdk/logging/export/BatchLogProcessor.java	`78.30% <0.00%> (-4.06%)`	⬇️
...entelemetry/exporter/otlp/internal/Serializer.java	`81.15% <0.00%> (-3.46%)`	⬇️
...dk/extension/trace/export/DisruptorEventQueue.java	`78.40% <0.00%> (-3.12%)`	⬇️
...lemetry/exporter/otlp/internal/JsonSerializer.java	`90.32% <0.00%> (-3.02%)`	⬇️
...sion/trace/jaeger/sampler/JaegerRemoteSampler.java	`81.39% <0.00%> (-2.33%)`	⬇️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c87e2b3...3887ef4. Read the comment docs.

jkwatson · 2021-09-27T17:11:11Z

...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java

+      String protocol = getOtlpProtocol(dataType, config);
+      if (endpoint != null
+          && protocol.equals(PROTOCOL_HTTP_PROTOBUF)
+          && !endpoint.endsWith(signalPath)) {


This seems like it precludes someone pointing at a totally different endpoint of their own choosing. I'm not sure that's a good thing, since perhaps their pointing at a proxy or some such that is doing the mapping to the actual path?

If we want to force the signal path like this, I would recommend that we use URI.resolve(), rather than hand-crafting the URL to use. If we use that method, it doesn't matter what they've put on the end of their host/port...we'll use the one we know is right.

But, again, this seems dangerous and that it would break anyone who is doing something not 100% standard with their ingest.

Perhaps the best thing would be to construct a URI, then see if it has a path component, and leave it alone if it does, otherwise resolve it to the required signal-specific path.

This only applies to otel.exporter.otlp.endpoint configuration. No modification will be made to otel.exporter.otlp.traces.endpoint or otel.exporter.otlp.metrics.endpoint configuration. This seems to be the intention of the spec given the wording:

The per-signal endpoint configuration options take precedence and can be used to override this behavior.

I'm happy to use URI to resolve the path. Did string concatenation because it was enough for the collector.

Hm. ok, I see how that probably makes sense since it probably wasn't really usable as-is, anyway. I think the URI.resolve() implementation is a little cleaner, since we don't have to worry about the trailing slashes, etc. Unless, of course, we think that people may want to pre-pend some sort of pre-path before the actual signal path? That's probably a very narrow edge case, though, and I'm not sure those people would have anything usable right now anyway.

Pushed up a commit using URI.resolve() instead of string concatenation. Personally, I prefer the string concatenation approach since URI required handling a checked exception and the behavior of .resolve(..) isn't super intuitive. Don't feel strongly about this.

I don't feel super strongly about it, either. Checking for validity seems like a positive thing from URI, though, doesn't it? And, yeah, resolve() is a bit weird around slash-handling (which I honestly didn't remember to be the case when I suggested it).

anuraaga · 2021-09-28T07:57:11Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

+    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");


Can you add a test for http://localhost:4317/collector/v1/traces -> http://localhost:4317/collector/v1/traces and http://localhost:4317/collector -> http://localhost:4317/collector/v1/traces ?

I think that would violate the spec. Configuring a per-signal endpoint should not append anything, so http://localhost:4317/collector should stay as just that. As worded in the spec, the "/v1/traces" suffix is merely convention for the collector. If I have a backend that does not follow this convention, I should be able to override the per-signal endpoint with the per-signal configuration, without having anything changed in the URL.

Oberon00

In the PR description and docs, you say you will append the per-signal path "if not present already". Do you mean "if the per-signal config is not already set"? If so, please clarify the wording.

Oberon00 · 2021-09-28T08:08:51Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

+    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");


I think that would violate the spec. Configuring a per-signal endpoint should not append anything, so http://localhost:4317/collector should stay as just that. As worded in the spec, the "/v1/traces" suffix is merely convention for the collector. If I have a backend that does not follow this convention, I should be able to override the per-signal endpoint with the per-signal configuration, without having anything changed in the URL.

Oberon00 · 2021-09-28T08:10:32Z

...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java

+          && !endpoint.endsWith(signalPath)) {
+        if (!endpoint.endsWith("/")) {
+          endpoint += "/";
+        }


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.

...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java

jack-berg · 2021-09-28T20:06:56Z

@Oberon00

In the PR description and docs, you say you will append the per-signal path "if not present already". Do you mean "if the per-signal config is not already set"? If so, please clarify the wording.

By "if not present already" I'm referring to the current wording of the spec which says that the signal specific path (i.e. v1/traces) is appended if otel.exporter.otlp.endpoint is specified and doesn't already end with v1/traces. The signal specific configuration (i.e. otel.exporter.otlp.traces.endpoint) still takes precedence over otel.exporter.otlp.endpoint.

I see you have a PR that changes the spec wording to say that the signal specific path MUST always be appended. That would change the behavior as follows:

current: otel.exporter.otlp.endpoint=http://localhost:4317/v1/traces => http://localhost:4317/v1/traces
as proposed: otel.exporter.otlp.endpoint=http://localhost:4317/v1/traces => http://localhost:4317/v1/traces/v1/traces

I think the java SDK follow the behavior as currently worded until your spec PR is merged. I'm happy to update the behavior when that occurs.

Oberon00 · 2021-09-28T21:46:53Z

What will the metric export URL be with the current behavior if I specify otel.exporter.otlp.endpoint=http://localhost:4317/v1/traces ? I would assume that metrics are then exported to http://localhost:4317/v1/traces which would make no sense, because if you only cared about traces you should just set the per-signal endpoint.

If we interpret the spec very literally, it could also be http://localhost:4317/v1/traces/v1/metrics (which it would be with the new behavior, making it perhaps more clear that the initial part should be omitted).

jack-berg · 2021-09-28T22:29:36Z

What will the metric export URL be with the current behavior if I specify otel.exporter.otlp.endpoint=http://localhost:4317/v1/traces ? I would assume that metrics are then exported to http://localhost:4317/v1/traces which would make no sense, because if you only cared about traces you should just set the per-signal endpoint.

This is the behavior of the main branch today.

If we interpret the spec very literally, it could also be http://localhost:4317/v1/traces/v1/metrics (which it would be with the new behavior, making it perhaps more clear that the initial part should be omitted).

This is the behavior of the PR as it currently stands.

jkwatson · 2021-09-28T23:18:47Z

What will the metric export URL be with the current behavior if I specify otel.exporter.otlp.endpoint=http://localhost:4317/v1/traces ? I would assume that metrics are then exported to http://localhost:4317/v1/traces which would make no sense, because if you only cared about traces you should just set the per-signal endpoint.

This is the behavior of the main branch today.

If we interpret the spec very literally, it could also be http://localhost:4317/v1/traces/v1/metrics (which it would be with the new behavior, making it perhaps more clear that the initial part should be omitted).

This is the behavior of the PR as it currently stands.

It sounds like we definitely need some spec clarification on this. :)

jack-berg · 2021-09-28T23:38:15Z

The commit I just pushed up now adds URL validation for endpoints that is more strict, but also more in inline with the spec, as I suggested I could do in this comment.

It also has test cases that show:

A variety of valid endpoints
A variety of invalid endpoints
How the signal path is appended to the endpoint if the protocol is http/protobuf in a variety of scenarios

I suggest that we don't add any more scope to this PR.

anuraaga · 2021-09-29T01:33:57Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

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


Thanks - this is a descriptive and good case to have here

The current wording of the OTLP endpoint config was confusing, especially the "if not present already" wording. Instead, clarify that we must always append when using the envvar for all signals (it was already clearly specified that the per-signal vars do not get the path appended). This came up in open-telemetry/opentelemetry-java#3650 and again at open-telemetry/opentelemetry-java#3666 (comment). Also make it a MUST (not SHOULD) since this kind of thing would be extremely annoying to have differences per-language in. Also, without appending, the variable cannot be used to configure more than one signal which would defeat its sole purpose.

jack-berg · 2021-10-04T20:14:19Z

Now that @Oberon00 's spec PR has been merged, I've updated this PR to always append the signal path when otel.exporter.otlp.endpoint is specified. This is reflected in the test cases as well.

jkwatson · 2021-10-05T15:32:30Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

+    assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces"))
+        .isEqualTo("http://localhost:4317/v1/traces/v1/traces");
+    assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/metrics"))
+        .isEqualTo("http://localhost:4317/v1/metrics/v1/traces");


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 /v1/traces when it's already on the end seems surprising to me. Yes, it's precisely what the user is requesting, but I suspect this will happen and users will complain. Are we were we don't want to check to make sure that the generic endpoint hasn't already had the /v1/<signal> tacked onto it before duplicating it?

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 /v1/traces/v1/metrics.
If you suggest that the configuration should be "smart" and detect any signal path (e.g., replacing /v1/metrics with /v1/traces for traces, leaving it in-place for metrics) I suggest a spec PR. I wouldn't be against it, if we can manage to log some warning at least...

Maybe I misunderstood and you meant the signal-specific thing? That seems like a bug indeed. I'll comment above. #3666 (comment)

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.

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).
I could be convinced of the "fully smart" method, stripping off any known trailing signal path from the generic URL, but making only the configured path work seems to be a bad balance.

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!

Oberon00 · 2021-10-05T15:53:36Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

+    assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces"))
+        .isEqualTo("http://localhost:4317/v1/traces/v1/traces");


Suggested change

assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces"))

.isEqualTo("http://localhost:4317/v1/traces/v1/traces");

assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces"))

.isEqualTo("http://localhost:4317/v1/traces");

The spec mandates that signal-specific URLs are left alone and /v1/traces is not appended. What if you have an endpoint that needs an URL not ending in that? I don't think the OTLP spec mandates a particular URL path.
EDIT: That aspect did not change through my PR BTW.

The per-signal endpoint configuration options take precedence and can be used to override this behavior (the URL is used as-is for them, without any modifications).

and

For the per-signal variables (OTEL_EXPORTER_OTLP__ENDPOINT), the URL MUST be used as-is without any modification. The only exception is that if an URL contains no path part, the root path / MUST be used (see Example 2).

https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp (and table above)

OK, maybe I misunderstand what the test does because the implementation at L56 in sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java looks like it's doing the right thing, not appending the signal path for the per-signal config.

yes, it's quite hard to figure out what cases are being tested in these tests, at least from the github diff.

Maybe a better function name could already help?

yes, maybe configureHttpProtobufEndpoint could be renamed to configureNonSignalSpecificEndpoint or something to make it super clear in the OtlpConfigUtilTest

OK, maybe I misunderstand what the test does because the implementation at L56 in sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java looks like it's doing the right thing, not appending the signal path for the per-signal config.

Yes these test cases are specifically testing the behavior of the signal agnostic otel.exporter.otlp.endpoint property.

I can try to improve the names of the function(s) to improve clarity.

Hmm, not formally blocking the PR because I might not have time to monitor it further. But please fix this.

jkwatson

With a CHANGELOG entry explaining the potential breakage, this is 👍🏽

…on generic http endpoint

jack-berg · 2021-10-05T19:38:50Z

...sions/autoconfigure/src/test/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtilTest.java

+  }
+
+  @Test
+  void configureOtlpExporterBuilder_HttpGenericEndpointKey() {


I've tried to improve the naming conventions in the tests. There are now 3 tests:

configureOtlpExporterBuilder_HttpGenericEndpointKey validates the behavior when the generic otel.exporter.otlp.endpoint is used to configure the http endpoint

configureOtlpExporterBuilder_HttpTracesEndpointKey validates the behavior when the trace specific otel.exporter.otlp.traces.endpoint is used to configure the http endpoint (included validating that it overrides the generic endpoint value)

configureOtlpExporterBuilder_HttpMetricsEndpointKey validates the behavior when the metrics specific otel.exporter.otlp.metrics.endpoint is used to configure the http endpoint (included validating that it overrides the generic endpoint value)

I think the method names are pretty good. In any case, they're better than repeating a ~7 line ImmutableMap.of(..) 20+ times. If its not immediately obvious what the configureEndpointForHttp does, take a quick look at it - IMO it greatly improves the readability of the test cases.

jkwatson · 2021-10-05T19:48:17Z

Thanks!

The current wording of the OTLP endpoint config was confusing, especially the "if not present already" wording. Instead, clarify that we must always append when using the envvar for all signals (it was already clearly specified that the per-signal vars do not get the path appended). This came up in open-telemetry/opentelemetry-java#3650 and again at open-telemetry/opentelemetry-java#3666 (comment). Also make it a MUST (not SHOULD) since this kind of thing would be extremely annoying to have differences per-language in. Also, without appending, the variable cannot be used to configure more than one signal which would defeat its sole purpose.

Update autoconfigure to append signal path to otlp http endpoint if n…

b3a1b7e

…ot present

jack-berg requested review from anuraaga, arminru, bogdandrutu, carlosalberto and jkwatson as code owners September 27, 2021 16:18

jack-berg requested a review from a user September 27, 2021 16:18

jack-berg requested review from Oberon00, pavolloffay, thisthat and tylerbenson as code owners September 27, 2021 16:18

jack-berg commented Sep 27, 2021

View reviewed changes

jkwatson reviewed Sep 27, 2021

View reviewed changes

Use URI.resolve() instead of string concatenation

0793d5f

anuraaga reviewed Sep 28, 2021

View reviewed changes

Oberon00 reviewed Sep 28, 2021

View reviewed changes

...xtensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/OtlpConfigUtil.java Outdated Show resolved Hide resolved

Oberon00 mentioned this pull request Sep 28, 2021

Clarify OTEL_EXPORTER_OTLP_ENDPOINT envvar for OTLP/HTTP. open-telemetry/opentelemetry-specification#1975

Merged

revert to string concatenation

58c8d2a

Add URL validation to OTLP endpoint

ce32b38

anuraaga approved these changes Sep 29, 2021

View reviewed changes

Always append signal path to otlp http/protobuf endpoint

1557491

jkwatson reviewed Oct 5, 2021

View reviewed changes

Oberon00 previously requested changes Oct 5, 2021

View reviewed changes

jkwatson approved these changes Oct 5, 2021

View reviewed changes

Add changelog entry, clarify tests, append root path if not included …

3887ef4

…on generic http endpoint

jack-berg commented Oct 5, 2021

View reviewed changes

jkwatson merged commit 5556795 into open-telemetry:main Oct 5, 2021

This was referenced Dec 19, 2021

Temurin JDK #4011

Merged

use Eclipse Temurin JDK docker image #4012

Merged

		assertThat(configureHttpProtobufEndpoint(DATA_TYPE_TRACES, "http://localhost:4317/v1/traces"))
		.isEqualTo("http://localhost:4317/v1/traces/v1/traces");

Update autoconfigure to append signal path to otlp http endpoint if n… #3666

Update autoconfigure to append signal path to otlp http endpoint if n… #3666

Conversation

jack-berg commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 27, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Oberon00 left a comment

Choose a reason for hiding this comment

Oberon00 Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jack-berg commented Sep 28, 2021

Oberon00 commented Sep 28, 2021 • edited Loading

jack-berg commented Sep 28, 2021

jkwatson commented Sep 28, 2021

jack-berg commented Sep 28, 2021

Choose a reason for hiding this comment

jack-berg commented Oct 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Oct 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkwatson commented Oct 5, 2021

codecov bot commented Sep 27, 2021 •

edited

Loading

Oberon00 Sep 28, 2021 •

edited

Loading

Oberon00 Sep 28, 2021 •

edited

Loading

Oberon00 commented Sep 28, 2021 •

edited

Loading

Oberon00 Oct 5, 2021 •

edited

Loading

Oberon00 Oct 5, 2021 •

edited

Loading