Update otel-exporter OTLP headers parsing to match format specs#1869
Update otel-exporter OTLP headers parsing to match format specs#1869codeboten merged 16 commits intoopen-telemetry:mainfrom
Conversation
- Edited unit tests to test that uppercase keys become lowercase and keys/values are trimmed of optional white spaces (OWS)
- Edited unit tests to test that uppercase keys become lowercase and keys/values are trimmed of optional white spaces (OWS)
…y-python into open-telemetry-main
…entelemetry-python into update-otlp-headers
eddyleelin
requested review from
a team,
owais and
srikanthccv
and removed request for
a team
...pentelemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py
Outdated
Show resolved
Hide resolved
| if isinstance(self._headers, str): | ||
| self._headers = tuple( | ||
| tuple(item.split("=")) for item in self._headers.split(",") | ||
| ) |
There was a problem hiding this comment.
Your change is totally fine but you could just add another list comprehension to the existing one. Something like
[(key.lower(), value) for key, value in [item.split("=") for item in headers.split(",")]]
since this runs only once, the extra pass over the collection should not be problem. That said, a plain loop may be more obvious/readable to some over nested comprehension. I'll leave it to you.
There was a problem hiding this comment.
I appreciate the feedback! I also thought about just adding another nested list comp, but it was getting a bit out of hand, so I made it into a plain loop! Thanks again 😄
| for header_pair in self._headers.split(","): | ||
| key, value = header_pair.split("=", maxsplit=1) | ||
| key = key.strip().lower() | ||
| value = value.strip() |
There was a problem hiding this comment.
I'm sure if we should strip individual keys and values. Generally we just strip a pair as people tend to add whitespace after comma but don't do it before/after =. That said, I can't think of any case where someone would want whitespace at the beginning or end of a key/value so I guess it's fine.
Co-authored-by: Owais Lone <owais@users.noreply.github.com>
ocelotl
left a comment
ocelotl
left a comment
There was a problem hiding this comment.
Nice! Please just fix lint, @eddyleelin:
tox -e lintsource .tox/lint/bin/activateblack .isort .deactivate
This should fix black and isort errors automatically. pylint may still fail, though.
codeboten
left a comment
codeboten
left a comment
There was a problem hiding this comment.
Please update the branch @eddyleelin
5 participants
Description
This PR fixes the way the otel Python Exporter parses OTEL_EXPORTER_OTLP_HEADERS to align with the otel exporter specs. Now, the headers, which are represented in a format matching the W3C Correlation-Context specs for HTTP header formats., are correctly parsed:
Fixes #1851 by casting key string to lowercase, and #1852 by correctly splitting on just the first equals sign
'='and removing optional white spaces (OWS).Type of change
How Has This Been Tested?
tox -e test-exporter-otlp-proto-grpcsuccessfully completes.Does This PR Require a Contrib Repo Change?
No.
Checklist: