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

SanitizeValue.sanitize_header_values() type signature is incorrect #2358

Closed
aabmass opened this issue Mar 20, 2024 · 2 comments · Fixed by #2361
Closed

SanitizeValue.sanitize_header_values() type signature is incorrect #2358

aabmass opened this issue Mar 20, 2024 · 2 comments · Fixed by #2361

Comments

@aabmass
Copy link
Member

aabmass commented Mar 20, 2024

          > **Subtlety:** [the docs](https://opentelemetry.io/docs/specs/semconv/http/http-spans/#:~:text=is%20not%20recommended.-,The%20attribute%20value%20MUST%20consist%20of%20either%20multiple%20header%20values%20as%20an%20array%20of%20strings%20or%20a%20single%2Ditem%20array%20containing%20a%20possibly%20comma%2Dconcatenated%20string%2C%20depending%20on%20the%20way%20the%20HTTP%20library%20provides%20access%20to%20headers.,-%5B2%5D%3A%20The) allow you to use either a list of header values where the keys are repeated or a single item list with the values comma separated.

In the case of WSGI request headers which come pre-joined, I think it makes sense to leave them comma separated. For WSGI response headers and ASGI overall, where we have access to repeated headers, it might make sense to use the list for the reasons you mentioned. I think that is the sprit of the spec when it says "depending on the way the HTTP library provides access to headers".

this would means changing the signature of numerous public methods
and, changing the docstring which perversely (presumably according to some old spec) specificly say you'll get a single item list with values comma seprated

@ocelotl @xrmx any thoughts on this?

I was actually confused reading the code, because It looks like this signature for SanitizeValue.sanitize_header_values() is already wrong (it's returning a dict[str, list[str]] 😓 Were there other signatures?

Originally posted by @aabmass in #2266 (review)

@samuelcolvin
Copy link
Contributor

Fix proposed in #2361.

@xrmx
Copy link
Contributor

xrmx commented Mar 20, 2024

Subtlety quote is:
The attribute value MUST consist of either multiple header values as an array of strings or a single-item array containing a possibly comma-concatenated string, depending on the way the HTTP library provides access to headers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants