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

Jump to bottom

Do not count skipped baggage entries #2079

Merged
merged 3 commits into from
Aug 31, 2021
Merged

Do not count skipped baggage entries #2079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a9b79a
Do not count skipped baggage entries
ocelotl Aug 30, 2021
75ae1bc
Add exception raising test
ocelotl Aug 31, 2021
88b5878
Fix lint
ocelotl Aug 31, 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 @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

- `opentelemetry-semantic-conventions` Update to semantic conventions v1.6.1
([#2077](https://github.com/open-telemetry/opentelemetry-python/pull/2077))
- Fix propagation bug caused by counting skipped entries
([#2071](https://github.com/open-telemetry/opentelemetry-python/pull/2071))

## [1.5.0-0.24b0](https://github.com/open-telemetry/opentelemetry-python/releases/tag/v1.5.0-0.24b0) - 2021-08-26

Expand Down
6 changes: 3 additions & 3 deletions opentelemetry-api/src/opentelemetry/baggage/propagation/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,6 @@ def extract(
baggage_entries = header.split(",")
total_baggage_entries = self._MAX_PAIRS
for entry in baggage_entries:
if total_baggage_entries <= 0:
return context
total_baggage_entries -= 1
if len(entry) > self._MAX_PAIR_LENGTH:
continue
try:
Expand All @@ -68,6 +65,9 @@ def extract(
unquote_plus(value).strip(),
context=context,
)
total_baggage_entries -= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as to why we are counting backwards as opposed to simply just counting to _MAX_PAIRS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 It was implemented like that, so I kept the implementation.

if total_baggage_entries == 0:
break

return context

Expand Down
40 changes: 40 additions & 0 deletions opentelemetry-api/tests/baggage/test_baggage_propagation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,46 @@ def test_extract_unquote_plus(self):
{"key/key": "value/value"},
)

def test_header_max_entries_skip_invalid_entry(self):

self.assertEqual(
self._extract(
",".join(
[
f"key{index}=value{index}"
if index != 2
else (
f"key{index}="
f"value{'s' * (W3CBaggagePropagator._MAX_PAIR_LENGTH + 1)}"
)
for index in range(W3CBaggagePropagator._MAX_PAIRS + 1)
]
)
),
{
f"key{index}": f"value{index}"
for index in range(W3CBaggagePropagator._MAX_PAIRS + 1)
if index != 2
},
)
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
self.assertEqual(
self._extract(
",".join(
[
f"key{index}=value{index}"
if index != 2
else f"key{index}xvalue{index}"
for index in range(W3CBaggagePropagator._MAX_PAIRS + 1)
]
)
),
{
f"key{index}": f"value{index}"
for index in range(W3CBaggagePropagator._MAX_PAIRS + 1)
if index != 2
},
)

def test_inject_no_baggage_entries(self):
values = {}
output = self._inject(values)
Expand Down