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.

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

Fix ASGIGetter keys to fetch from actual carrier headers #1435

Merged
merged 14 commits into from
Jan 12, 2023
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- `opentelemetry-instrumentation-aws-lambda` Adds an option to configure `disable_aws_context_propagation` by
environment variable: `OTEL_LAMBDA_DISABLE_AWS_CONTEXT_PROPAGATION`
([#1507](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1507))
- `opentelemetry-instrumentation-asgi` Fix keys() in class ASGIGetter to correctly fetch values from carrier headers.
([#1435](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1435))


## Version 1.15.0/0.36b0 (2022-12-10)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,8 @@ def get(
return decoded

def keys(self, carrier: dict) -> typing.List[str]:
return [_key.decode("utf8") for (_key, _value) in carrier]
headers = carrier.get("headers") or []
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, if "headers" is not in carrier.keys, shouldn't this method return [_key.decode("utf8") for (_key, _value) in carrier]. The way this is being modified makes this method return an empty list if "headers" is not in carrier.keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering the .get() method above returns header-specific fields and its description mentions "header name in scope", I assumed here we'd only want to return headers rather than the whole carrier, which in this case is the ASGI scope. I'm not too sure fetching all values from the scope is correct though, there's a bunch of ASGI-specific info there and I wouldn't expect to see (nor inject) any extra info from Opentelemetry.

Now that you mention it though, I guess some people may be relying on this behaviour now, so maybe we should join both lists? Or would we treat this as a breaking change instead?

Copy link

@torqataNate torqataNate Dec 17, 2022

Choose a reason for hiding this comment

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

I would argue that this should be keyed to headers, but we do need to be conscious of other formats. For example as we look through most proprogators, the common use is for the carrier to only contain header keys. The most relatable part of the spec docs are here.
https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#:~:text=In%20order%20to%20increase%20compatibility%2C
The carrier of propagated data on both the client (injector) and server (extractor) side is usually an HTTP request. In order to increase compatibility, the key/value pairs MUST only consist of US-ASCII characters that make up valid HTTP header fields as per [RFC 7230](https://tools.ietf.org/html/rfc7230#section-3.2).

The http request will have other keys with it such as client, method, query params etc which we dont care about, we only need the headers. It is only the TextMapPropagator section that states the carrier is usually a http request though even though that does seem to be the standard to supply the http payload as a carrier. As we look at the spec on proprogators, It actually doesn't specify whats in a carrier. Which is here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/context/api-propagators.md#carrier

In the branch i was going to make a pull request against i searched for the headers key first and iderated over that only, and if that did not exist, I chose the carrier payload as it should then be a header.

The argument could be made that we need to look through the entire carrier payload for headers, as the format of carrier has not been specified. But it feels burdensome for this method to have to deal with that.

Lastly
The above pull request would also fail on line 264 when a header isn't a byte type so it is still broken. I had a pull request i was going to submit but im not a contributor. how would you like me to submit the proposed change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as this processor is concerned, it has been working until this change. The previous behaviour was not correct either, but at least it would not crash. The changes in this PR directly address the main issue, which is the crash due to iterating the wrong value, and fetching the correct value the same way we do in .get(). I would argue that any more changes in the behaviour than that are out of scope and probably require a separate discussion and investigation. In any case, something for the maintainers to decide CC @ocelotl @srikanthccv

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a correct fix both for the crash and behaviour because the web framework instrumentation may receive carriers of different types, but their goal is to inject/extract the trace context information into the HTTP header, which is used for propagation. And since the ASGI specification says the headers is Iterable[[byte string, byte string]] I think it's safe to assume keys will be a byte string so it's not broken.

return [_key.decode("utf8") for (_key, _value) in headers]


asgi_getter = ASGIGetter()
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
import typing
from unittest import mock

import opentelemetry.instrumentation.asgi as otel_asgi
from opentelemetry import trace
from opentelemetry.context import Context
from opentelemetry.propagate import get_global_textmap, set_global_textmap
from opentelemetry.propagators.textmap import (
CarrierT,
Getter,
Setter,
TextMapPropagator,
default_getter,
default_setter,
)
from opentelemetry.test.asgitestutil import AsgiTestBase
from opentelemetry.test.test_base import TestBase
from opentelemetry.trace import SpanKind
Expand All @@ -13,6 +25,60 @@
from .test_asgi_middleware import simple_asgi


class MockTextMapPropagator(TextMapPropagator):
"""Mock propagator for testing purposes using both getter `get` and `all`."""

TRACE_ID_KEY = "mock-traceid"
SPAN_ID_KEY = "mock-spanid"

def extract(
self,
carrier: CarrierT,
context: typing.Optional[Context] = None,
getter: Getter = default_getter,
) -> Context:
if context is None:
context = Context()

trace_id_list = getter.get(carrier, self.TRACE_ID_KEY)
span_id_list = getter.get(carrier, self.SPAN_ID_KEY)
carrier_keys = getter.keys(carrier)

if not trace_id_list or not span_id_list:
assert not any(key in carrier_keys for key in self.fields)
return context

assert all(key in carrier_keys for key in self.fields)
return trace.set_span_in_context(
trace.NonRecordingSpan(
trace.SpanContext(
trace_id=int(trace_id_list[0]),
span_id=int(span_id_list[0]),
is_remote=True,
)
),
context,
)

def inject(
self,
carrier: CarrierT,
context: typing.Optional[Context] = None,
setter: Setter = default_setter,
) -> None:
span = trace.get_current_span(context)
setter.set(
carrier, self.TRACE_ID_KEY, str(span.get_span_context().trace_id)
)
setter.set(
carrier, self.SPAN_ID_KEY, str(span.get_span_context().span_id)
)

@property
def fields(self):
return {self.TRACE_ID_KEY, self.SPAN_ID_KEY}


async def http_app_with_custom_headers(scope, receive, send):
message = await receive()
assert scope["type"] == "http"
Expand All @@ -34,6 +100,8 @@ async def http_app_with_custom_headers(scope, receive, send):
b"my-custom-regex-value-3,my-custom-regex-value-4",
),
(b"my-secret-header", b"my-secret-value"),
(MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"),
(MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"),
],
}
)
Expand All @@ -60,6 +128,8 @@ async def websocket_app_with_custom_headers(scope, receive, send):
b"my-custom-regex-value-3,my-custom-regex-value-4",
),
(b"my-secret-header", b"my-secret-value"),
(MockTextMapPropagator.TRACE_ID_KEY.encode(), b"1"),
(MockTextMapPropagator.SPAN_ID_KEY.encode(), b"2"),
],
}
)
Expand Down Expand Up @@ -88,6 +158,11 @@ def setUp(self):
self.app = otel_asgi.OpenTelemetryMiddleware(
simple_asgi, tracer_provider=self.tracer_provider
)
self.previous_propagator = get_global_textmap()
set_global_textmap(MockTextMapPropagator())

def tearDown(self):
set_global_textmap(self.previous_propagator)
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't follow what the tests were targeting. I would test that ASGIGetter instance doesn't throw an exception for scope dict, which is currently failing, and additionally assert that keys return the Headers keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea was to ensure that the values returned by get() and keys() were the same and they were consistent across different tests, but it was a bit of a hack indeed with that custom propagator. I added simpler tests in test_getter.py instead that indeed fail before the fix and pass now, hope they make sense c219bb0


def test_http_custom_request_headers_in_span_attributes(self):
self.scope["headers"].extend(
Expand Down