Skip to content
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

Not clear whether values in OTEL_EXPORTER_OTLP_HEADERS should be URL encoded #3832

Closed
pyohannes opened this issue Jan 18, 2024 · 10 comments
Closed
Assignees
Labels
area:configuration Related to configuring the SDK [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:protocol Related to the specification/protocol directory

Comments

@pyohannes
Copy link
Contributor

What are you trying to achieve?

We're trying to share the same values for OTEL_EXPORTER_OTLP_HEADERS across different languages. However, while Python requires header values to be URL encoded, Java and .NET cannot deal with URL encoded values. I didn't yet check other languages.

Additional context.

The specification refers to the W3C Baggage specification for details about the format of OTEL_EXPORTER_OTLP_HEADERS:

The OTEL_EXPORTER_OTLP_HEADERS, OTEL_EXPORTER_OTLP_TRACES_HEADERS, OTEL_EXPORTER_OTLP_METRICS_HEADERS, OTEL_EXPORTER_OTLP_LOGS_HEADERS environment variables will contain a list of key value pairs, and these are expected to be represented in a format matching to the W3C Baggage, except that additional semi-colon delimited metadata is not supported, i.e.: key1=value1,key2=value2. All attribute values MUST be considered strings.

The W3C Baggage spec requires values to be URL encoded:

A value contains a string whose character encoding MUST be UTF-8 [Encoding]. Any characters outside of the baggage-octet range of characters MUST be percent-encoded. Characters which are not required to be percent-encoded MAY be percent-encoded.

I wonder whether it was the original intent of requiring URL encoding for OTEL_EXPORTER_OTLP_HEADERS, or whether that's an unintended side-effect of referencing the W3C Baggage spec.

@jack-berg
Copy link
Member

I think the spec is clear and its just a bug in the case of java not dealing with URL encoded values. For proof, consider that the resource specification contains similar language for how key / values are encoded: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/resource/sdk.md#specifying-resource-information-via-an-environment-variable

And in otel java, we perform url decoding on resource attribute values: https://github.com/open-telemetry/opentelemetry-java/blob/main/sdk-extensions/autoconfigure/src/main/java/io/opentelemetry/sdk/autoconfigure/ResourceConfiguration.java#L70

We should just do the same thing for OTEL_EXPORTER_OTLP_HEADERS.

@pyohannes
Copy link
Contributor Author

pyohannes commented Jan 19, 2024

I think the spec is clear and its just a bug in the case of java not dealing with URL encoded values.

Looking at different exporter implementations, it seems that implications of referring to W3C Baggage aren't all that clear:

Language URL-encoded OTEL_EXPORTER_OTLP_HEADERS non URL-encoded OTEL_EXPORTER_OTLP_HEADERS
.NET ✔️
.C++ ✔️
Go ✔️ ✔️
Java ✔️
JS ✔️ ✔️
Python ✔️
PHP ✔️
Rust ✔️
Ruby ✔️ ✔️

What's not clear to me is whether the desired behavior is the one implemented in Go (both encoded and non-encoded headers are supported) or the one implemented in Python (only encoded headers are supported).

I assume that languages currently not supporting encoded headers will do what Go does and support both, as otherwise this would be a breaking change.

Note that this isn't an esoteric use case, as authentication headers contain spaces.

@arminru arminru added the area:configuration Related to configuring the SDK label Jan 23, 2024
@dyladan
Copy link
Member

dyladan commented Jan 23, 2024

JS also url-decodes OTEL_EXPORTER_OTLP_HEADERS

@pyohannes
Copy link
Contributor Author

@dyladan But JS still accepts non URL-encoded values in OTEL_EXPORTER_OTLP_HEADERS (like Go does)? It doesn't strictly require proper URL-encoding like Python?

@jack-berg
Copy link
Member

I assume that languages currently not supporting encoded headers will do what Go does and support both, as otherwise this would be a breaking change.

I'm interpreting this as a bug fix.

@dyladan
Copy link
Member

dyladan commented Jan 23, 2024

It does not strictly require it in JS

@pyohannes
Copy link
Contributor Author

I'm interpreting this as a bug fix.

Agreed. However, it's not clear to me how best to fix it in a consistent ways across languages.

Two possibilities proposed today in the spec meeting:

  1. Only properly URL-encoded values should be supported. This will potentially break users of all languages except Python who currently use non URL-encoded values.
  2. Support both URL-encoded and non URL-encoded values (which is not strictly required by the spec). This will not break users. If it's decided to go that route, it might be worth thinking about requiring this by the spec.

@jack-berg
Copy link
Member

jack-berg commented Jan 23, 2024

👍

FWIW, in the proposed java fix for this, I just added a test to confirm that non-url encoded values continue to work as well. So with the fix, java would join js and go in supporting both URL-encoded and non-URL-encoded.

@reyang reyang added the [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR label Jan 24, 2024
@pyohannes
Copy link
Contributor Author

  1. Support both URL-encoded and non URL-encoded values (which is not strictly required by the spec).

This seems to be the feasible way to go. If there are no objections, I can create PRs for languages currently not supporting URL encoded values to:

  • Add support for URL encoded header values to conform with the specification.
  • Keep support for non URL encoded values to not break users.

shakuzen pushed a commit to micrometer-metrics/micrometer that referenced this issue Jan 31, 2024
brettmc added a commit to brettmc/opentelemetry-php that referenced this issue Feb 28, 2024
Per a recent spec clarification in open-telemetry/opentelemetry-specification#3832 OTLP
headers should support url-encoded header values.
brettmc added a commit to open-telemetry/opentelemetry-php that referenced this issue Feb 28, 2024
Per a recent spec clarification in open-telemetry/opentelemetry-specification#3832 OTLP
headers should support url-encoded header values.

Co-authored-by: Tobias Bachert <[email protected]>
otel-php-bot pushed a commit to opentelemetry-php/exporter-otlp that referenced this issue Feb 28, 2024
Per a recent spec clarification in open-telemetry/opentelemetry-specification#3832 OTLP
headers should support url-encoded header values.

Co-authored-by: Tobias Bachert <[email protected]>
@pyohannes
Copy link
Contributor Author

PRs for implementing the solution described in #3832 (comment) have been submitted for .NET, Java, PHP, Rust, and C++. For Erlang and Swift I don't know the current state of support of URL encoded values, however, I raised this issue in the relevant Slack channels.

Given this, I'll close this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:configuration Related to configuring the SDK [label deprecated] triaged-accepted [label deprecated] Issue triaged and accepted by OTel community, can proceed with creating a PR spec:protocol Related to the specification/protocol directory
Projects
None yet
Development

No branches or pull requests

6 participants