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

Develop/condition server span django #832

Merged
merged 23 commits into from
Jan 7, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
f82f1d1
Code changes and pytests for https://github.com/open-telemetry/opente…
sanketmehta28 Dec 14, 2021
73fdd4c
removing unnecessary imports
sanketmehta28 Dec 14, 2021
7dffc0a
removing unnecessary imports
sanketmehta28 Dec 14, 2021
e169ce4
adding wsgi.py file to get the wsgi application object
sanketmehta28 Dec 14, 2021
b216e6b
Revert "Updating personal fork from public repo"
sanketmehta28 Dec 15, 2021
522fff4
Merge branch 'main' into develop/condition_server_span_django
sanketmehta28 Dec 15, 2021
08d8fce
Revert "Updating personal fork from public repo"
sanketmehta28 Dec 15, 2021
52e53a3
Merge branch 'develop/condition_server_span_django' of github.com:san…
sanketmehta28 Dec 15, 2021
74681e8
Revert "Updating personal fork from public repo"
sanketmehta28 Dec 15, 2021
3a6f77b
Revert "Updating personal fork from public repo"
sanketmehta28 Dec 15, 2021
f316f95
Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…
sanketmehta28 Dec 21, 2021
c601bdc
Changing the unit test case by removing WSGI instrumentation and make…
sanketmehta28 Dec 27, 2021
93f41c6
removing unnecessary import statements
sanketmehta28 Dec 27, 2021
764e9f0
Revert "Updating personal fork from public repo"
sanketmehta28 Dec 15, 2021
bd2491f
Merge branch 'main' into develop/condition_server_span_django
sanketmehta28 Jan 5, 2022
0af9168
resolving failed builds for lint and generate
sanketmehta28 Jan 5, 2022
7016dbc
Merge branch 'develop/condition_server_span_django' of github.com:san…
sanketmehta28 Jan 6, 2022
c63ac73
removing commented code
sanketmehta28 Jan 6, 2022
c4d503a
removing blank line
sanketmehta28 Jan 6, 2022
d689edf
removed unused variable resp from test_middleware.py and modified th…
sanketmehta28 Jan 6, 2022
f3986ab
modified the CHANGELOG.md to removed unnecessary entry
sanketmehta28 Jan 6, 2022
76dfec3
modified the CHANGELOG.md to add proper PR entry
sanketmehta28 Jan 6, 2022
0932f6e
Merge branch 'main' into develop/condition_server_span_django
ocelotl Jan 7, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,13 @@
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
from opentelemetry.trace import (
INVALID_SPAN,
Span,
SpanKind,
get_current_span,
use_span,
)
from opentelemetry.util.http import get_excluded_urls, get_traced_request_attrs

try:
Expand Down Expand Up @@ -183,11 +189,16 @@ def process_request(self, request):
carrier_getter = wsgi_getter
collect_request_attributes = wsgi_collect_request_attributes

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

token = context = None
span_kind = SpanKind.INTERNAL
if get_current_span() is INVALID_SPAN:
context = extract(request_meta, getter=wsgi_getter)
Copy link
Contributor

Choose a reason for hiding this comment

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

earlier we were using carrier_getter which could be either a wsgi or asgi getter. Now we are always using wsgi_getter. is this intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh my bad. corrected to carrier_getter.

token = attach(context)
span_kind = SpanKind.SERVER
span = self._tracer.start_span(
self._get_span_name(request),
kind=SpanKind.SERVER,
context,
kind=span_kind,
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
start_time=request_meta.get(
"opentelemetry-instrumentor-django.starttime_key"
),
Expand Down Expand Up @@ -220,7 +231,8 @@ def process_request(self, request):

request.META[self._environ_activation_key] = activation
request.META[self._environ_span_key] = span
request.META[self._environ_token] = token
if token:
request.META[self._environ_token] = token
Copy link
Contributor

Choose a reason for hiding this comment

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

since this is conditional now, do we need to update places where this is being read to make sure everthing continues to work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes Without this check, It was raising an exception while detaching the token from request object

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean earlier we were always setting this value in the dict even if it was None but now we won't be setting it at all in some cases. This can result in lookup error if code in other places assumes the key will always be present so we should make sure this doesn't break anything else.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a check before detaching the token


if _DjangoMiddleware._otel_request_hook:
_DjangoMiddleware._otel_request_hook( # pylint: disable=not-callable
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,13 @@
from unittest.mock import Mock, patch

from django import VERSION, conf
from django.core.wsgi import get_wsgi_application
from django.core.servers.basehttp import get_internal_wsgi_application
from django.http import HttpRequest, HttpResponse
from django.test.client import Client
from django.test.utils import setup_test_environment, teardown_test_environment
from django.test.client import Client, RequestFactory
from django.test.testcases import SimpleTestCase
from django.test.utils import override_settings, setup_test_environment, teardown_test_environment
from opentelemetry import trace

from opentelemetry.instrumentation.django import (
DjangoInstrumentor,
Expand All @@ -30,6 +34,7 @@
TraceResponsePropagator,
set_global_response_propagator,
)
from opentelemetry.instrumentation.wsgi import OpenTelemetryMiddleware
from opentelemetry.sdk import resources
from opentelemetry.sdk.trace import Span
from opentelemetry.sdk.trace.id_generator import RandomIdGenerator
Expand Down Expand Up @@ -409,6 +414,7 @@ def setUp(self):
result = self.create_tracer_provider(resource=resource)
tracer_provider, exporter = result
self.exporter = exporter
self.tracer_provider = tracer_provider
_django_instrumentor.instrument(tracer_provider=tracer_provider)

def tearDown(self):
Expand All @@ -432,3 +438,22 @@ def test_tracer_provider_traced(self):
self.assertEqual(
span.resource.attributes["resource-key"], "resource-value"
)

def test_django_with_wsgi_instrumented(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine but it would be nicer if we could test this in a generic way without wsgi instrumentation. At least lets add a comment documenting what this test does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure I will add the comment.
Also what do you mean by generic way? can you please give me an example?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking of just injecting a span into the execution context so the instrumentation would find it and trigger the code path. That way the test wouldn't depend on wsgi instrumentation but it is not a big issue for me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Please do check and provide your comments.

application = get_wsgi_application()
application = OpenTelemetryMiddleware(application, tracer_provider=self.tracer_provider)
environ = RequestFactory()._base_environ(
PATH_INFO="/span_name/1234/",
CONTENT_TYPE="text/html; charset=utf-8",
REQUEST_METHOD="GET"
)
response_data = {}
def start_response(status, headers):
response_data["status"] = status
response_data["headers"] = headers

resp = next(application(environ, start_response))
srikanthccv marked this conversation as resolved.
Show resolved Hide resolved
span_list = self.exporter.get_finished_spans()
self.assertEqual(span_list[0].attributes[SpanAttributes.HTTP_STATUS_CODE], 200)
self.assertEqual(trace.SpanKind.INTERNAL, span_list[0].kind)
self.assertEqual(trace.SpanKind.SERVER, span_list[1].kind)