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

Add Django ASGI support #391

Merged
merged 9 commits into from
Oct 12, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 5 additions & 3 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#706](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/706))
- `opentelemetry-instrumentation-requests` added exclude urls functionality
([#714](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/714))
- `opentelemetry-instrumentation-django` Add ASGI support
([#391](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/391))

### Changed
- `opentelemetry-instrumentation-botocore` Make common span attributes compliant with semantic conventions
Expand Down Expand Up @@ -64,12 +66,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Added
- `opentelemetry-sdk-extension-aws` Add AWS resource detectors to extension package
([#586](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/586))
- `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`,
`opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks
- `opentelemetry-instrumentation-asgi`, `opentelemetry-instrumentation-aiohttp-client`, `openetelemetry-instrumentation-fastapi`,
`opentelemetry-instrumentation-starlette`, `opentelemetry-instrumentation-urllib`, `opentelemetry-instrumentation-urllib3` Added `request_hook` and `response_hook` callbacks
([#576](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/576))
- `opentelemetry-instrumentation-pika` added RabbitMQ's pika module instrumentation.
([#680](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/680))

### Changed

- `opentelemetry-instrumentation-fastapi` Allow instrumentation of newer FastAPI versions.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ def get_host_port_url_tuple(scope):
"""Returns (host, port, full_url) tuple."""
server = scope.get("server") or ["0.0.0.0", 80]
port = server[1]
server_host = server[0] + (":" + str(port) if port != 80 else "")
server_host = server[0] + (":" + str(port) if str(port) != "80" else "")
ocelotl marked this conversation as resolved.
Show resolved Hide resolved
full_path = scope.get("root_path", "") + scope.get("path", "")
http_url = scope.get("scheme", "http") + "://" + server_host + full_path
return server_host, port, http_url
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ install_requires =
opentelemetry-semantic-conventions == 0.24b0

[options.extras_require]
asgi =
opentelemetry-instrumentation-asgi == 0.24b0
test =
opentelemetry-test == 0.24b0

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

import types
from logging import getLogger
from time import time
from typing import Callable
Expand All @@ -24,11 +25,11 @@
get_global_response_propagator,
)
from opentelemetry.instrumentation.utils import extract_attributes_from_object
from opentelemetry.instrumentation.wsgi import add_response_attributes
from opentelemetry.instrumentation.wsgi import (
add_response_attributes,
collect_request_attributes,
wsgi_getter,
collect_request_attributes as wsgi_collect_request_attributes,
)
from opentelemetry.instrumentation.wsgi import wsgi_getter
from opentelemetry.propagate import extract
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.trace import Span, SpanKind, use_span
Expand All @@ -43,6 +44,7 @@
from django.urls import Resolver404, resolve

DJANGO_2_0 = django_version >= (2, 0)
DJANGO_3_0 = django_version >= (3, 0)

if DJANGO_2_0:
# Since Django 2.0, only `settings.MIDDLEWARE` is supported, so new-style
Expand All @@ -67,6 +69,26 @@ def __call__(self, request):
except ImportError:
MiddlewareMixin = object

if DJANGO_3_0:
from django.core.handlers.asgi import ASGIRequest
adamantike marked this conversation as resolved.
Show resolved Hide resolved
else:
ASGIRequest = None

# try/except block exclusive for optional ASGI imports.
try:
adamantike marked this conversation as resolved.
Show resolved Hide resolved
from opentelemetry.instrumentation.asgi import asgi_getter
from opentelemetry.instrumentation.asgi import (
collect_request_attributes as asgi_collect_request_attributes,
)
from opentelemetry.instrumentation.asgi import set_status_code

_is_asgi_supported = True
except ImportError:
asgi_getter = None
asgi_collect_request_attributes = None
set_status_code = None
_is_asgi_supported = False


_logger = getLogger(__name__)
_attributes_by_preference = [
Expand All @@ -91,6 +113,10 @@ def __call__(self, request):
]


def _is_asgi_request(request: HttpRequest) -> bool:
return ASGIRequest is not None and isinstance(request, ASGIRequest)
adamantike marked this conversation as resolved.
Show resolved Hide resolved


class _DjangoMiddleware(MiddlewareMixin):
"""Django Middleware for OpenTelemetry"""

Expand Down Expand Up @@ -140,12 +166,25 @@ def process_request(self, request):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
return

is_asgi_request = _is_asgi_request(request)
if not _is_asgi_supported and is_asgi_request:
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Can a django service/middleware handle both WSGI and ASGI requests at the same time? If it can only handle one kind, may be this can can run in middleware init and result can be stored in an instance variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.

Copy link
Contributor Author

@adamantike adamantike Sep 21, 2021

Choose a reason for hiding this comment

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

Can a django service/middleware handle both WSGI and ASGI requests at the same time?

Yes, it can handle both, so we need to check which kind of request we're receiving on each call.

At least is_asgi_supported can be called before _is_asgi_request and that way we can avoid running the type check for non-ASGI requests.

As we're using is_asgi_request multiple times on this method, I don't think we can avoid the type-check call.

  • If ASGI is not supported, we need to know if the current request is ASGI.
  • If ASGI is supported, we need to know it nevertheless, to decide on which carrier, carrier_getter, etc. to use.


# pylint:disable=W0212
request._otel_start_time = time()

request_meta = request.META

token = attach(extract(request_meta, getter=wsgi_getter))
if is_asgi_request:
carrier = request.scope
carrier_getter = asgi_getter
collect_request_attributes = asgi_collect_request_attributes
else:
carrier = request_meta
carrier_getter = wsgi_getter
collect_request_attributes = wsgi_collect_request_attributes

token = attach(extract(request_meta, getter=carrier_getter))

span = self._tracer.start_span(
self._get_span_name(request),
Expand All @@ -155,12 +194,25 @@ def process_request(self, request):
),
)

attributes = collect_request_attributes(request_meta)
attributes = collect_request_attributes(carrier)

if span.is_recording():
attributes = extract_attributes_from_object(
request, self._traced_request_attrs, attributes
)
if is_asgi_request:
# ASGI requests include extra attributes in request.scope.headers.
attributes = extract_attributes_from_object(
types.SimpleNamespace(
**{
name.decode("latin1"): value.decode("latin1")
for name, value in request.scope.get("headers", [])
}
),
self._traced_request_attrs,
attributes,
)

for key, value in attributes.items():
span.set_attribute(key, value)

Expand Down Expand Up @@ -207,15 +259,22 @@ def process_response(self, request, response):
if self._excluded_urls.url_disabled(request.build_absolute_uri("?")):
return response

is_asgi_request = _is_asgi_request(request)
if not _is_asgi_supported and is_asgi_request:
return response

activation = request.META.pop(self._environ_activation_key, None)
span = request.META.pop(self._environ_span_key, None)

if activation and span:
add_response_attributes(
span,
f"{response.status_code} {response.reason_phrase}",
response,
)
if is_asgi_request:
Copy link
Contributor

Choose a reason for hiding this comment

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

does add_response_attributes only set a status code? The name suggests it does more than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does actually. It adds HTTP status code attribute in addition to setting status on the span. Plus is also has some additional logic which we miss out on here. Any reason we cannot just use that function for ASGI as well?

def add_response_attributes(
span, start_response_status, response_headers
): # pylint: disable=unused-argument
"""Adds HTTP response attributes to span using the arguments
passed to a PEP3333-conforming start_response callable."""
if not span.is_recording():
return
status_code, _ = start_response_status.split(" ", 1)
try:
status_code = int(status_code)
except ValueError:
span.set_status(
Status(
StatusCode.ERROR,
"Non-integer HTTP status: " + repr(status_code),
)
)
else:
span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
span.set_status(Status(http_status_to_status_code(status_code)))

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 the functions are right now, I don't see any needed changes for both WSGI and ASGI to make the same changes to the current span:

  • WSGI:
    def add_response_attributes(
    span, start_response_status, response_headers
    ): # pylint: disable=unused-argument
    """Adds HTTP response attributes to span using the arguments
    passed to a PEP3333-conforming start_response callable."""
    if not span.is_recording():
    return
    status_code, _ = start_response_status.split(" ", 1)
    try:
    status_code = int(status_code)
    except ValueError:
    span.set_status(
    Status(
    StatusCode.ERROR,
    "Non-integer HTTP status: " + repr(status_code),
    )
    )
    else:
    span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
    span.set_status(Status(http_status_to_status_code(status_code)))
  • ASGI:
    def set_status_code(span, status_code):
    """Adds HTTP response attributes to span using the status_code argument."""
    if not span.is_recording():
    return
    try:
    status_code = int(status_code)
    except ValueError:
    span.set_status(
    Status(
    StatusCode.ERROR,
    "Non-integer HTTP status: " + repr(status_code),
    )
    )
    else:
    span.set_attribute(SpanAttributes.HTTP_STATUS_CODE, status_code)
    span.set_status(Status(http_status_to_status_code(status_code)))

set_status_code(span, response.status_code)
Copy link
Contributor

Choose a reason for hiding this comment

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

HTTP status codes need to be translated to OpenTelemetry status codes before setting the span status e.g,

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 also think this is already fixed in the current set_status_code implementation.

else:
add_response_attributes(
span,
f"{response.status_code} {response.reason_phrase}",
response,
)

propagator = get_global_response_propagator()
if propagator:
Expand All @@ -238,7 +297,7 @@ def process_response(self, request, response):
activation.__exit__(None, None, None)

if self._environ_token in request.META.keys():
detach(request.environ.get(self._environ_token))
detach(request.META.get(self._environ_token))
request.META.pop(self._environ_token)

return response
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,11 @@
from sys import modules
from unittest.mock import Mock, patch

from django import VERSION
from django.conf import settings
from django.conf.urls import url
from django import VERSION, conf
from django.http import HttpRequest, HttpResponse
from django.test import Client
from django.test.client import Client
from django.test.utils import setup_test_environment, teardown_test_environment
from django.urls import re_path

from opentelemetry.instrumentation.django import (
DjangoInstrumentor,
Expand Down Expand Up @@ -57,22 +56,22 @@
DJANGO_2_2 = VERSION >= (2, 2)

urlpatterns = [
url(r"^traced/", traced),
url(r"^route/(?P<year>[0-9]{4})/template/$", traced_template),
url(r"^error/", error),
url(r"^excluded_arg/", excluded),
url(r"^excluded_noarg/", excluded_noarg),
url(r"^excluded_noarg2/", excluded_noarg2),
url(r"^span_name/([0-9]{4})/$", route_span_name),
re_path(r"^traced/", traced),
re_path(r"^route/(?P<year>[0-9]{4})/template/$", traced_template),
re_path(r"^error/", error),
re_path(r"^excluded_arg/", excluded),
re_path(r"^excluded_noarg/", excluded_noarg),
re_path(r"^excluded_noarg2/", excluded_noarg2),
re_path(r"^span_name/([0-9]{4})/$", route_span_name),
]
_django_instrumentor = DjangoInstrumentor()


class TestMiddleware(TestBase, WsgiTestBase):
@classmethod
def setUpClass(cls):
conf.settings.configure(ROOT_URLCONF=modules[__name__])
super().setUpClass()
settings.configure(ROOT_URLCONF=modules[__name__])

def setUp(self):
super().setUp()
Expand Down Expand Up @@ -105,6 +104,11 @@ def tearDown(self):
teardown_test_environment()
_django_instrumentor.uninstrument()

@classmethod
def tearDownClass(cls):
super().tearDownClass()
conf.settings = conf.LazySettings()

def test_templated_route_get(self):
Client().get("/route/2020/template/")

Expand Down Expand Up @@ -357,6 +361,7 @@ def test_trace_response_headers(self):
class TestMiddlewareWithTracerProvider(TestBase, WsgiTestBase):
@classmethod
def setUpClass(cls):
conf.settings.configure(ROOT_URLCONF=modules[__name__])
super().setUpClass()

def setUp(self):
Expand All @@ -375,6 +380,11 @@ def tearDown(self):
teardown_test_environment()
_django_instrumentor.uninstrument()

@classmethod
def tearDownClass(cls):
super().tearDownClass()
conf.settings = conf.LazySettings()

def test_tracer_provider_traced(self):
Client().post("/traced/")

Expand Down
Loading