Add support for regular expression matching and sanitizing of headers in ASGI. #1333
Conversation
| custom_headers.strip() | ||
| for custom_headers in custom_headers.split(",") | ||
| ] | ||
| if custom_headers.strip().lower() == "all": |
There was a problem hiding this comment.
This requires regular expression matching of headers to work. I'll be adding this to one instrumentation at a time (ASGI in this one).
In theory, if all the instrumentations don't get updated in the same release, this could cause problems if someone is capturing a header named All, but I think the odds of that are low.
There was a problem hiding this comment.
Even if the odds are low, I suggest not having this all option. Since the regex already does the job, I would add a note on how to capture all the headers in the doc.
|
@srikanthccv Could you please take a look at this? |
rogersd
force-pushed
the
drogers-wildcard-headers
branch
from
0b2f186
to
263b5aa
Compare
| ] | ||
| if not decoded: | ||
| return None | ||
| return decoded | ||
|
|
||
| def keys(self, carrier: dict) -> typing.List[str]: | ||
| return list(carrier.keys()) | ||
| return [ | ||
| _key.decode("utf8") for (_key, _value) in carrier.get("headers") |
There was a problem hiding this comment.
The carrier here is expected to be headers dict so the carrier.keys() should be fine. If the caller is incorrectly using then I think fix should be implemented there.
There was a problem hiding this comment.
Ok, I left the decode() in but I took the other part out. Let me know if you want that part removed too.
| custom_headers.strip() | ||
| for custom_headers in custom_headers.split(",") | ||
| ] | ||
| if custom_headers.strip().lower() == "all": |
There was a problem hiding this comment.
Even if the odds are low, I suggest not having this all option. Since the regex already does the job, I would add a note on how to capture all the headers in the doc.
| custom_request_headers_regex_compiled = re.compile( | ||
| "|".join("^" + i + "$" for i in custom_request_headers_name), | ||
| re.IGNORECASE, | ||
| ) | ||
|
|
||
| for header_name in list( | ||
| filter( | ||
| custom_request_headers_regex_compiled.match, | ||
| asgi_getter.keys(scope), | ||
| ) | ||
| ): | ||
| header_values = asgi_getter.get(scope, header_name.lower()) | ||
| if header_values: | ||
| key = normalise_request_header_name(header_name.lower()) | ||
| attributes[key] = [ | ||
| sanitize.sanitize_header_value( | ||
| header=header_name, value=header_values[0] | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Could you add this to util-http package so that it will be easier to use in other instrumentation?
rogersd
force-pushed
the
drogers-wildcard-headers
branch
3 times, most recently
from
95b881d
to
29ed503
Compare
… in ASGI. Next part of open-telemetry#1184 Details: Add test cases for regular expression matching. Add test cases for header sanitizing. Add documentation for regular expression matching and header sanitation. Various documentation cleanups and standardization. Fix keys() in class ASGIGetter so it returns the HTTP header keys instead of a list of available request data. This makes it consistent with the WSGIGetter keys() method. Make ASGIGetter.get() compare all keys in a case insensitive manner. Move TestCustomHeaders to a separate file to avoid module length error.
rogersd
force-pushed
the
drogers-wildcard-headers
branch
from
29ed503
to
6c4ad5f
Compare
|
@srikanthccv I think this should be ready to go, sorry for the delay! |
|
@srikanthccv Thanks for the review! May you please merge this when you have a minute so that I can open the equivalent PR for WGSI? Thanks! |
Next part of #1184
Description
Add test cases for regular expression matching.
Add test cases for "all" keyword.
Add test cases for header sanitizing.
Add documentation for regular expression matching and header sanitation.
Various documentation cleanups and standardization.
Fix keys() in class ASGIGetter so it returns the HTTP header keys instead of a list of available request data. This makes it consistent with the WSGIGetter keys() method.
Make ASGIGetter.get() compare all keys in a case insensitive manner.
Fixes #1184
Type of change
How Has This Been Tested?
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.