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.

Sign up for GitHub

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

Update otel-exporter OTLP headers parsing to match format specs #1869

Merged
merged 16 commits into from
May 26, 2021
Merged

Update otel-exporter OTLP headers parsing to match format specs #1869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
85af83e
Update OTLP header parsing in exporter
eddyleelin May 14, 2021
01eb50c
Update CHANGELOG.md
eddyleelin May 14, 2021
d421626
Updated exporter otlp header parsing
eddyleelin May 18, 2021
d6ef97a
Updated exporter otlp header parsing
eddyleelin May 18, 2021
d8e1367
Added initial changelog entry stub
eddyleelin May 18, 2021
ad281f6
Fixed error in appending header tuples
eddyleelin May 18, 2021
c64dd22
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
eddyleelin May 18, 2021
dfe8d9e
Merge branch 'open-telemetry-main' into update-otlp-headers
eddyleelin May 18, 2021
4f33b06
Added Changed header to change log entry
eddyleelin May 20, 2021
cd0fdf4
Removed extraspacing for pylint
eddyleelin May 20, 2021
eac6146
Merge branch 'update-otlp-headers' of https://github.com/open-o11y/op…
eddyleelin May 24, 2021
daf9d13
Update CHANGELOG.md
eddyleelin May 24, 2021
4b67a7d
Merge branch 'main' into update-otlp-headers
eddyleelin May 25, 2021
578c4a7
Updated tuple to literal
eddyleelin May 25, 2021
1d81888
Fixed lint error
eddyleelin May 26, 2021
b287ab0
Merge branch 'main' into update-otlp-headers
eddyleelin May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#1854](https://github.com/open-telemetry/opentelemetry-python/pull/1854))
- Changed AttributeValue sequences to warn mypy users on adding None values to array
([#1855](https://github.com/open-telemetry/opentelemetry-python/pull/1855))
- Fixed exporter OTLP header parsing to match baggage header formatting.
([#1869](https://github.com/open-telemetry/opentelemetry-python/pull/1869))

## [1.2.0, 0.21b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.2.0-0.21b0) - 2021-05-11

Expand Down
17 changes: 14 additions & 3 deletions ...telemetry-exporter-otlp-proto-grpc/src/opentelemetry/exporter/otlp/proto/grpc/exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,9 +229,20 @@ def __init__(

self._headers = headers or environ.get(OTEL_EXPORTER_OTLP_HEADERS)
if isinstance(self._headers, str):
self._headers = tuple(
tuple(item.split("=")) for item in self._headers.split(",")
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

temp_headers = []
for header_pair in self._headers.split(","):
key, value = header_pair.split("=", maxsplit=1)
key = key.strip().lower()
value = value.strip()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

temp_headers.append(
(
key,
value,
)
)

self._headers = tuple(temp_headers)

self._timeout = timeout or int(
environ.get(OTEL_EXPORTER_OTLP_TIMEOUT, 10)
)
Expand Down
8 changes: 4 additions & 4 deletions exporter/opentelemetry-exporter-otlp-proto-grpc/tests/test_otlp_trace_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ def tearDown(self):
OTEL_EXPORTER_OTLP_TRACES_ENDPOINT: "collector:4317",
OTEL_EXPORTER_OTLP_TRACES_CERTIFICATE: THIS_DIR
+ "/fixtures/test.cert",
OTEL_EXPORTER_OTLP_TRACES_HEADERS: "key1=value1,key2=value2",
OTEL_EXPORTER_OTLP_TRACES_HEADERS: " key1=value1,KEY2 = value=2",
OTEL_EXPORTER_OTLP_TRACES_TIMEOUT: "10",
OTEL_EXPORTER_OTLP_TRACES_COMPRESSION: "gzip",
},
Expand All @@ -195,7 +195,7 @@ def test_env_variables(self, mock_exporter_mixin):
_, kwargs = mock_exporter_mixin.call_args_list[0]

self.assertEqual(kwargs["endpoint"], "collector:4317")
self.assertEqual(kwargs["headers"], "key1=value1,key2=value2")
self.assertEqual(kwargs["headers"], " key1=value1,KEY2 = value=2")
self.assertEqual(kwargs["timeout"], 10)
self.assertEqual(kwargs["compression"], Compression.Gzip)
self.assertIsNotNone(kwargs["credentials"])
Expand All @@ -217,7 +217,7 @@ def test_no_credentials_error(

@patch.dict(
"os.environ",
{OTEL_EXPORTER_OTLP_TRACES_HEADERS: "key1=value1,key2=value2"},
{OTEL_EXPORTER_OTLP_TRACES_HEADERS: " key1=value1,KEY2 = VALUE=2 "},
)
@patch(
"opentelemetry.exporter.otlp.proto.grpc.exporter.ssl_channel_credentials"
Expand All @@ -228,7 +228,7 @@ def test_otlp_headers_from_env(self, mock_ssl_channel, mock_secure):
exporter = OTLPSpanExporter()
# pylint: disable=protected-access
self.assertEqual(
exporter._headers, (("key1", "value1"), ("key2", "value2"))
exporter._headers, (("key1", "value1"), ("key2", "VALUE=2"))
)
exporter = OTLPSpanExporter(
headers=(("key3", "value3"), ("key4", "value4"))
Expand Down