Fix OtlpExporterOptionsExtensions.GetHeaders incorrectly throwing for comma in header value

[tobias-tengler]

Fixes #6510

Changes

Updates OtlpExporterOptionsExtensions.GetHeaders to no longer throw for commas inside of header values. Previously a valid header value like VL-Stream-Fields=service.name,service.environment would've thrown, since the comma would've been incorrectly interpreted as a delimiter to another header pair - now it's properly parsed.

Merge requirement checklist

• [x] CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• [x] Unit tests added/updated
• [x] Appropriate CHANGELOG.md files updated for non-trivial changes
• [x] Changes in public API reviewed (if applicable)

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: tobias-tengler / name: Tobias Tengler (226edef3306205a6888f8af244fc190b06d6160d, f0f44e6841b656c3b51607610e4e348dcd3041af, 4e1b47142e8d41159242670cdbb996844e5e27b7)

[tebeco]

I feel like the headers shouldn't be a string at all TBH. the day we'll need to send a Base64 value that's being padded with = what will happen ?

If we're sending headerS (multiples), it sounds like this should be a List<KeyValuePair<string, string>> or something similar @matt-hensley . Just like Resource/Attribute

I'm not mentionning Dictionary<string, string> because i'm not sure it's good or not to deal with dup / de-dup in headers, I don't know the specs enough

Else it will turn into another footgun TBH

[tobias-tengler]

@tebeco I like the idea. Just want to add that we for example define the headers as an environment variable that we pass to the string option, since we use different exporters for different environments. If the API would change to just List<KeyValuePair<string, string>> we would have to do this parsing in user-land.

Which could be fine. It would just add churn for everyone currently using the string option.

[tebeco]

the other thing i haven't looked at all is performance i don't know the impact of switching from string to List

what i remember is in aspnetcore they use a special StringValues value type because generally a header often only have one value

i have no knowledge of the OTEL specifics here

im just adding it here in case it rings a bell for someone

i hope it doesn't explode the scope of that evol'

[tebeco]

@tebeco I like the idea.

Just want to add that we for example define the headers as an environment variable that we pass to the string option, since we use different exporters for different environments.

If the API would change to just List<KeyValuePair<string, string>> we would have to do this parsing in user-land.

Which could be fine. It would just add churn for everyone currently using the string option.

yeah, i feel the pain, dealing with env var and array is generally not fun

unless you split env var to multiple aggregate yourself and then map it similar to what dotnet config does for array (but that's not pretty)

i don't think there's such thing as escape in env var and its also OS specific i assume

[tobias-tengler]

@tebeco @matt-hensley what would you recommend as next steps? Should I implement the headers as List<KeyValuePair<string, string>> or how should we proceed here. This is currently blocking me and I haven't been able to find a workaround.

[tebeco]

this is something one of the maintainers of this repository should properly plan ahead to avoid a later API issue

I'm just an outsider here with the similar issue, but from the code itself, not from env var

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[rajkumar-rangaraj]

this is something one of the maintainers of this repository should properly plan ahead to avoid a later API issue

I'm just an outsider here with the similar issue, but from the code itself, not from env var

Agreed, this is something that warrants proper planning from the maintainers. We've been focused on other high priority items in the repo and will take a closer look at this after we complete the .NET 10 tasks.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[tebeco]

can someone check how to remove the auto stale bot as well as the comment spammed above ?

this feature is still missing and blocking vendor requiring vendor that needs multi header value. this can be useful when integrated the solution with external partners

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.

[tobias-tengler]

this is something one of the maintainers of this repository should properly plan ahead to avoid a later API issue I'm just an outsider here with the similar issue, but from the code itself, not from env var

Agreed, this is something that warrants proper planning from the maintainers. We've been focused on other high priority items in the repo and will take a closer look at this after we complete the .NET 10 tasks.

@rajkumar-rangaraj Seeing that a version with support for .NET 10 has been released, do you think the maintainers could have a look at this in the near future?

[tebeco]

this is indeed an important topic for vendor integration

[tobias-tengler]

@rajkumar-rangaraj good catch. I can come up with a fix and update my PR, if we want to keep this string-based API. Previous comments raised the issue that if we touch this part we should probably align to a key-value-pair based API like it's done in other parts. Either is fine with me, I just don't want to waste time on something we might discard again before merging.

[rajkumar-rangaraj]

@rajkumar-rangaraj good catch. I can come up with a fix and update my PR, if we want to keep this string-based API. Previous comments raised the issue that if we touch this part we should probably align to a key-value-pair based API like it's done in other parts. Either is fine with me, I just don't want to waste time on something we might discard again before merging.

Given this isn't in the hot path, the most pragmatic approach might be to keep it simpler. That said, it's good to apply performance/zero-allocation patterns.

[tebeco]

i don't want to disrupt but why a special treatment on "," or others

would this be naive to have another property that defines the delimiter and split on it ?

this assume that this delimiter is now the delimiters for headers

and is defaulted to ","

[rajkumar-rangaraj]

i don't want to disrupt but why a special treatment on "," or others

would this be naive to have another property that defines the delimiter and split on it ?

this assume that this delimiter is now the delimiters for headers

and is defaulted to ","

We follow a principle of minimizing the number of public APIs that perform similar tasks. Hence this is not an option.

[tebeco]

fair enough, this decision only weight into maintenance cost between a parser and all possible combinatory string including futures one (dealing with " ' ` or or \" or "" or nesting ...), versus just splitting on a const

I totally understand the decision made regarding the "api surface" in balance of all that 👍

[github-actions[bot]]

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or pushing will instruct the bot to automatically remove the label. This bot runs once per day.