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

Reimplement the keys method in WSGI CarrierGetter #379

Merged
merged 3 commits into from
Mar 25, 2021
Merged

Reimplement the keys method in WSGI CarrierGetter #379

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
5eb6174
Reimplement the keys method in WSGI CarrierGetter
Mar 25, 2021
427c2c6
Update CHANGELOG.md accordingly
Mar 25, 2021
164b247
Merge branch 'main' into wsgi-carrier-keys
Mar 25, 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 @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#364](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/364))
- `opentelemetry-propagator-ot-trace` Do not throw an exception when headers are not present
([#378](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/378))
- `opentelemetry-instrumentation-wsgi` Reimplement `keys` method to return actual keys from the carrier instead of an empty list.
([#379](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/379))

### Changed
- Rename `IdsGenerator` to `IdGenerator`
Expand Down
8 changes: 7 additions & 1 deletion ...ion/opentelemetry-instrumentation-wsgi/src/opentelemetry/instrumentation/wsgi/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ def hello():
from opentelemetry.trace.status import Status, StatusCode

_HTTP_VERSION_PREFIX = "HTTP/"
_CARRIER_KEY_PREFIX = "HTTP_"
_CARRIER_KEY_PREFIX_LEN = len(_CARRIER_KEY_PREFIX)


class CarrierGetter(DictGetter):
Expand All @@ -89,7 +91,11 @@ def get(
return None

def keys(self, carrier):
return []
return [
key[_CARRIER_KEY_PREFIX_LEN:].lower().replace("_", "-")
for key in carrier
if key.startswith(_CARRIER_KEY_PREFIX)
]
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, did this prevent or limit propagation in some way when it was implemented as an empty list?

Copy link
Contributor Author

@marcinzaremba marcinzaremba Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It prevents any use case when information to be extracted is not known beforehand (for example HTTP headers starting with a particular prefix, but with unknown exact names). The great example is OT Trace Propagator, where the current implementation (empty list) renders injecting baggage unusable:

opentelemetry-python-contrib/propagator/opentelemetry-propagator-ot-trace/src/opentelemetry/propagators/ot_trace/__init__.py

Line 92 in f8e51c4

for key in getter.keys(carrier):

On the other hand when carrier would be manipulated directly, it would shift a specific implementation of keys translation (from uppercase, environ-like WSGI format) to every consumer.

Copy link
Contributor

Choose a reason for hiding this comment

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

The great example is OT Trace Propagator, where the current implementation (empty list) renders injecting baggage unusable:

Thanks. This is what I was interested in.



carrier_getter = CarrierGetter()
Expand Down
17 changes: 16 additions & 1 deletion instrumentation/opentelemetry-instrumentation-wsgi/tests/test_getter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,30 @@ def test_get_none(self):
getter = CarrierGetter()
carrier = {}
val = getter.get(carrier, "test")

self.assertIsNone(val)

def test_get_(self):
def test_get(self):
getter = CarrierGetter()
carrier = {"HTTP_TEST_KEY": "val"}
val = getter.get(carrier, "test-key")

self.assertEqual(val, ["val"])

def test_keys(self):
getter = CarrierGetter()
keys = getter.keys(
{
"HTTP_TEST_KEY": "val",
"HTTP_OTHER_KEY": 42,
"NON_HTTP_KEY": "val",
}
)

self.assertEqual(keys, ["test-key", "other-key"])

def test_keys_empty(self):
getter = CarrierGetter()
keys = getter.keys({})

self.assertEqual(keys, [])