-
Notifications
You must be signed in to change notification settings - Fork 621
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
Add fallback decoding for asgi headers #2837
Add fallback decoding for asgi headers #2837
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: why do we want a fallback strategy to latin-1 for a wrong encoded value?
And if we want it we also want tests |
ASGI docs kinda suggests these should be ASCII but that's not mandatory so being less picky may be a good idea: headers: These are byte strings of the exact byte sequences sent by the client/to be sent by the server. While modern HTTP standards say that headers should be ASCII, older ones did not and allowed a wider range of characters. Frameworks/applications should decode headers as they deem appropriate. https://asgi.readthedocs.io/en/latest/specs/www.html#wsgi-encoding-differences |
Was following the previous PR and discussion here. Would a different approach be better? (Ex: try-catch the decode and pass-through) |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
@rocky-ken we need some tests for this to get in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current tests don't cover the changes in ASGIGetter, right? I think would be nice to have tests for ASGIGetter too.
instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_custom_headers.py
Show resolved
Hide resolved
Not familiar with the ASGIGetter tests but I added some stuff so let me know if that covers it or not |
@rocky-ken they are under test_getter.py |
Description
Add fallback decoding for asgi headers if utf-8 decoding throws an error.
Duplicate of PR 1713 to hopefully get it over the finish line
Fixes #1814
Fixes #1478
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test it in asgi
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.