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 4 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 @@ -184,11 +190,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 @@ -221,7 +232,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 @@ -16,9 +16,12 @@
from unittest.mock import Mock, patch

from django import VERSION, conf
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 @@ -28,6 +31,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 @@ -401,10 +405,9 @@ def setUpClass(cls):
def setUp(self):
super().setUp()
setup_test_environment()
resource = resources.Resource.create(
{"resource-key": "resource-value"}
)
result = self.create_tracer_provider(resource=resource)
_django_instrumentor.instrument()

result = self.create_tracer_provider()
tracer_provider, exporter = result
self.exporter = exporter
_django_instrumentor.instrument(tracer_provider=tracer_provider)
Expand All @@ -430,3 +433,67 @@ def test_tracer_provider_traced(self):
self.assertEqual(
span.resource.attributes["resource-key"], "resource-value"
)

class TestDjangoWithOtherFramework(SimpleTestCase, TestBase, WsgiTestBase):
@classmethod
def setUpClass(cls):
conf.settings.configure(ROOT_URLCONF=modules[__name__], WSGI_APPLICATION="tests.wsgi.application")
super().setUpClass()

def setUp(self):
super().setUp()
setup_test_environment()

# conf.settings.WSGI_APPLICATION = "wsgi.application"
# application = get_internal_wsgi_application()
# application = get_wsgi_application()
# _DjangoMiddleware
# self.application = OpenTelemetryMiddleware(application)
# _django_instrumentor.instrument()
# self.application = OpenTelemetryMiddleware(application, tracer_provider=self.tracer_provider)
# conf.settings.configure(ROOT_URLCONF=modules[__name__], WSGI_APPLICATION="application")
# conf.settings.WSGI_APPLICATION="application"


def tearDown(self) -> None:
super().tearDown()
teardown_test_environment()
_django_instrumentor.uninstrument()

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


def test_with_another_framework(self):
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

result = self.create_tracer_provider()
tracer_provider, exporter = result
self.exporter = exporter


_django_instrumentor.instrument(tracer_provider=tracer_provider)
application = get_internal_wsgi_application()
application = OpenTelemetryMiddleware(application, tracer_provider=tracer_provider)
resp = application(environ, start_response)

# resp = Client().get("/span_name/1234/")
# self.assertEqual(200, resp.status_code)

# span_list = self.memory_exporter.get_finished_spans()
span_list = self.exporter.get_finished_spans()
# print(span_list)
self.assertEqual(trace.SpanKind.INTERNAL, span_list[0].kind)

#Below line give me error "index out of the range" as there is only one span created where it should be 2.
Copy link
Member

Choose a reason for hiding this comment

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

It appears that your server span from middleware started but never ended. Please try changing the line 488 to next(application(environ, start_response)) and check.

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. Done and changes are pushed. Please do review them

Copy link
Member

Choose a reason for hiding this comment

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

I only intend to suggest that hack to validate the hypothesis. You should use test client instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

With Client() it was not creating the WSGI span. That is the reason I have to go this way

self.assertEqual(trace.SpanKind.SERVER, span_list[1].kind)
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
from django.core.wsgi import get_wsgi_application
from opentelemetry.instrumentation.django import DjangoInstrumentor
from opentelemetry.instrumentation.wsgi import OpenTelemetryMiddleware

application = get_wsgi_application()