Add: opentelemetry-ext-asgi

[Skeen]

This change implements an ASGI counterpart to the WSGI extension.

It has been tested with django channels on a dummy project here: https://github.com/Skeen/django_opentelemetry_example

WSGI Jaeger trace:

ASGI Jaeger trace:

ASGI websocket Jaeger trace:

[toumorokoshi]

Nice! This looks really thorough, and from a code standpoint, all looks really good.

I think there's a couple areas from a design standpoint that's worth talking through. I've added comments above but:

1. can we share more with ext-wsgi? duplicate code can become hard to maintain, especially as we begin to spilt codebases out of this one.
2. the choice for the 4 spans per request will be fairly noisy. How does this look in practice? Is this how all tracing ASGI implementations work today?

[toumorokoshi]

looking at the conversation in the opentelemetry-specification, we should use the HTTP method rather than the specialized path, since it has very high cardinality: https://github.com/open-telemetry/opentelemetry-specification/pull/416/files.

But this is a bug with the existing wsgi implementation as well.

[toumorokoshi]

Here's a tracking ticket for the wsgi implementation, but it'd be good to fix it in ASGI now: #409

[Skeen]

Acknowledged, I'll change it to utilize the method_name.

Do you feel like I should make a similar change in the WSGI implementation to keep them equivalent, or would you rather have that change seperately?

[joshuahlang]

It appears the recent update to the specification includes

Instrumentation MUST NOT default to using URI
path as span name, but MAY provide hooks to allow custom logic to override the
default span name.

Might I suggest adding an optional callback to this middleware to allow clients to specify a override the default span name?

[Skeen]

I'll add an optional callback to this, and pass it through.
I will make a similar change to the WSGI implementation in another PR, just to keep them syncronized.

[Skeen]

Resolved in: ede28b2

[Skeen]

WSGI equivalent: #440

[toumorokoshi]

It feels like we should share this with the wsgi implementation somehow. Would it be bad if we just imported methods from the wsgi implementation, and made those more general?

[Skeen]

I was considering this myself, and had a version which imported this function from the WSGI extension instead of keeping it here, as it is untouched and thus identical.

My argument against this, and why I changed it back, is as this would introduce a dependency between the two implementation, and I did not feel comfortable with that with regards to packaging and shipping the package.

Thus, if I was to share code between the modules, I'd argue to refractor out common functionality into a utilities module, but I'm not sure where to put that.

As for sharing the get_header_from_x functionality, it would simply require a function transforming WSGI headers to ASGI headers or vice versa.

• We can make this change, if that's something we want.

[toumorokoshi]

Yeah, I see less of a rationale for get_headers_form_x since it requires a conversion of wsgi headers (although I could see us abstracting that out with a constructor).

I feel like MAYBE this is something we can put in the API. But would like maybe a second opinion on that.

[Skeen]

Alright, I'll leave the get_headers_from_x alone.

Do you think I should refactor out the http_status_to_canonical_code to a seperate module? - say opentelemetry-ext-httputil similar to the opentelemetry-ext-testutil? - or would we rather just deal with code-duplication or interdependencies between ext-asgi and ext-wsgi?

[joshuahlang]

My two cents: I'd vote for a separate module, or even add it to the SDK (new ext-sdk?) The OTEL spec defines this mapping explicitly, so may as well provide it in the SDK right?

[joshuahlang]

Could use this pattern to exclude python versions prior to 3.7 in coverage.

I suspect you'll also want to update the tox.ini to exclude py3{4,5,6} from the environments this module's tests are run under.

[Skeen]

Could use this pattern to exclude python versions prior to 3.7 in coverage.

I suspect you'll also want to update the tox.ini to exclude py3{4,5,6} from the environments this module's tests are run under.

I'm actually in the process of doing this right now.

[toumorokoshi]

@Skeen @majorgreys is picking up finishing this PR in #716. If you would like to come back and finish this up yourself, please ping myself or @majorgreys.

[Skeen]

Hi @toumorokoshi, @majorgreys. I've been wanting to get back to this, but I haven't found the time. I'm sorry. By all means feel free to take the work and finish it up for me.