OTEL_EXPORTER_OTLP_ENDPOINT treatment not following spec #200
Comments
|
TBH I've had a hard time understanding the nitpicking in that part of the spec. If you think you understand how it should be, please do send a PR. It likely won't break anyone and I agree it's best to stick with the spec. Getting this right will probably also help with adding metrics & logs someday too. |
Closed
tobert
pushed a commit
that referenced
this issue
May 25, 2023
Adds 2 tests to the regression suite to validate endpoints on general get /v1/traces appended, and that signal endpoints do not.
|
LGTM! Meant to take a stab at this last week but things have been hectic on my end, thank you for taking it on. |
tobert
added a commit
that referenced
this issue
May 26, 2023
* add TracesEndpoint field to Config * add Endpoint and EndpointSource fields to diagnostics * add * matching to Diagnostics checks, shorten output on failures Modified checkProcess to return a bool if the process met the expected conditions. If it failed unexpectedly, checkAll will bail out of running further checks on the data, so output is a lot less spammy. Added "*" support to the check on diagnostics data strings. * add diagnostics fields, add tests for #200 Adds 2 tests to the regression suite to validate endpoints on general get /v1/traces appended, and that signal endpoints do not. * add --traces-endpoint option * refactor endpoint parsing to match OTel specs Moved URL parsing to its own func, moving some code from the http/grpc option paths to it, as well as starting to consider the "source" of general (e.g. --endpoint, OTEL_EXPORTER_OTLP_ENDPOINT) or a signal (e.g. --signal-endpoint or OTEL_EXPORTER_OTLP_TRACES_ENDPOINT). They are treated differently now to match the spec. Added tests for parseEndpoint. * automatically set port for bare hostname + gRPC e.g. "localhost", "127.0.0.1" are always going to go to gRPC, and when no port is provided, should default to 4317. Might need to check the spec. It probably doesn't allow for bare hostname but otel-cli will because it should be easy to type. * update tests to account for default port on bare hostnames
|
release v0.3.0, please reopen if there's still an issue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
See the Protocol Exporter doc:
Fairly sure the issue is here:
This contradicts Example 3 in the spec:
otel-cliwill instead try sending the traces tohttp://collector:4318/mycollector, as the path is not empty. The collector I was sending traces to has a non-empty path in its URL, so this led to a bit of confusion.Easy enough for us to work around it by giving the trace URL to
otel-cli, but this is probably going to surprise other folk.Unsure how this can be fixed without a breaking change (even if it's arguably a bug), hence bringing it up for discussion instead of trying to PR directly.
The text was updated successfully, but these errors were encountered: