Develop/condition server span django #832

sanketmehta28 · 2021-12-14T16:45:02Z

Description

there may be other instrumented components that wrap an instrumented web framework such as WSGI. In such cases WSGI would already have generated a server span and used the remote span as parent. If Django did the same, traces would not make a lot of sense. Depending on whether a remote span is present in the incoming request's carrier, Django and WSGI spans would be completely different traces or be siblings instead of parent and child.

Fixes # #448

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

I have tested it locally using a sample app with django and wsgi instrumentation enabled with jaeger exporter.
I have sent a request to the app and observe on jaeger that there are two span created: WSGI span as SERVER span and Django span as INTERNEL span.

Does This PR Require a Core Repo Change?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Unit tests have been added

linux-foundation-easycla · 2021-12-14T16:45:10Z

The committers are authorized under a signed CLA.

✅ Sanket Mehta (f82f1d1, 73fdd4c, 7dffc0a)

sanketmehta28 · 2021-12-14T16:55:41Z

I need some input in the pytest I have added in this PR.
I have enabled django as well as WSGI instrumentation in TestDjangoWithOtherFramework class but I am getting only 1 span when I call self.exporter.get_finished_spans().

Strange thing is the span I am getting is of type INTERNEL and it has parent_id available. It means it does have a root span of type SERVER.

Unfortunately I could not find a way to get the root span info and test is getting failed in the last statement:
self.assertEqual(trace.SpanKind.SERVER, span_list[1].kind)

Can someone help me on this?

srikanthccv · 2021-12-15T01:07:32Z

instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py

+        # 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.


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.

Sure. Done and changes are pushed. Please do review them

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

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

…ketmehta28/opentelemetry-python-contrib into develop/condition_server_span_django

sanketmehta28 · 2021-12-15T18:57:25Z

changes are made according to Srikanth's comments. Unit tests are running fine now. Please do review the PR

owais · 2021-12-16T07:29:46Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

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


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?

Oh my bad. corrected to carrier_getter.

owais · 2021-12-16T07:31:54Z

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

@@ -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


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

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

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.

Added a check before detaching the token

owais · 2021-12-16T07:32:43Z

instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py

@@ -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):


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.

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

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.

Done. Please do check and provide your comments.

…y-python-contrib into develop/condition_server_span_django

CHANGELOG.md

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

… it from generalised

.../opentelemetry-instrumentation-django/src/opentelemetry/instrumentation/django/middleware.py

instrumentation/opentelemetry-instrumentation-django/tests/test_middleware.py

srikanthccv

Please resolve the conflicts and address the nit suggestion. LGTM

…ketmehta28/opentelemetry-python-contrib into develop/condition_server_span_django

CHANGELOG.md

…e CHANGELOG.md with PR entry

CHANGELOG.md

sanketmehta28 added 3 commits December 14, 2021 20:20

Code changes and pytests for #448

f82f1d1

removing unnecessary imports

73fdd4c

removing unnecessary imports

7dffc0a

sanketmehta28 requested a review from a team December 14, 2021 16:45

adding wsgi.py file to get the wsgi application object

e169ce4

srikanthccv reviewed Dec 15, 2021

View reviewed changes

sanketmehta28 and others added 4 commits December 16, 2021 00:23

Revert "Updating personal fork from public repo"

b216e6b

Merge branch 'main' into develop/condition_server_span_django

522fff4

Revert "Updating personal fork from public repo"

08d8fce

Merge branch 'develop/condition_server_span_django' of github.com:san…

52e53a3

…ketmehta28/opentelemetry-python-contrib into develop/condition_server_span_django

owais reviewed Dec 16, 2021

View reviewed changes

sanketmehta28 and others added 3 commits December 20, 2021 20:45

Revert "Updating personal fork from public repo"

74681e8

Revert "Updating personal fork from public repo"

3a6f77b

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

f316f95

…y-python-contrib into develop/condition_server_span_django

srikanthccv reviewed Dec 21, 2021

View reviewed changes

sanketmehta28 added 2 commits December 28, 2021 04:50

Changing the unit test case by removing WSGI instrumentation and make…

c601bdc

… it from generalised

removing unnecessary import statements

93f41c6

lzchen approved these changes Jan 3, 2022

View reviewed changes

srikanthccv reviewed Jan 4, 2022

View reviewed changes

Revert "Updating personal fork from public repo"

764e9f0

srikanthccv approved these changes Jan 5, 2022

View reviewed changes

sanketmehta28 and others added 5 commits January 5, 2022 19:03

Merge branch 'main' into develop/condition_server_span_django

bd2491f

resolving failed builds for lint and generate

0af9168

Merge branch 'develop/condition_server_span_django' of github.com:san…

7016dbc

…ketmehta28/opentelemetry-python-contrib into develop/condition_server_span_django

removing commented code

c63ac73

removing blank line

c4d503a

srikanthccv suggested changes Jan 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

sanketmehta28 added 2 commits January 6, 2022 19:06

removed unused variable resp from test_middleware.py and modified th…

d689edf

…e CHANGELOG.md with PR entry

modified the CHANGELOG.md to removed unnecessary entry

f3986ab

srikanthccv reviewed Jan 6, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

modified the CHANGELOG.md to add proper PR entry

76dfec3

sanketmehta28 requested a review from srikanthccv January 6, 2022 14:10

srikanthccv approved these changes Jan 6, 2022

View reviewed changes

Merge branch 'main' into develop/condition_server_span_django

0932f6e

ocelotl merged commit d01efe5 into open-telemetry:main Jan 7, 2022

sanketmehta28 mentioned this pull request Mar 7, 2022

REQUEST: New membership for sanketmehta28 open-telemetry/community#995

Closed

6 tasks

sanketmehta28 mentioned this pull request Apr 19, 2022

Issue to track Sanket's work for contrib approver #1053

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop/condition server span django #832

Develop/condition server span django #832

sanketmehta28 commented Dec 14, 2021 •

edited

Loading

linux-foundation-easycla bot commented Dec 14, 2021 •

edited

Loading

sanketmehta28 commented Dec 14, 2021

srikanthccv Dec 15, 2021

sanketmehta28 Dec 15, 2021

srikanthccv Dec 15, 2021

sanketmehta28 Dec 16, 2021

sanketmehta28 commented Dec 15, 2021

owais Dec 16, 2021

sanketmehta28 Dec 16, 2021

owais Dec 16, 2021

sanketmehta28 Dec 16, 2021

owais Dec 16, 2021

sanketmehta28 Dec 20, 2021

owais Dec 16, 2021

sanketmehta28 Dec 16, 2021

owais Dec 21, 2021

sanketmehta28 Dec 27, 2021

srikanthccv left a comment

Develop/condition server span django #832

Develop/condition server span django #832

Conversation

sanketmehta28 commented Dec 14, 2021 • edited Loading

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

linux-foundation-easycla bot commented Dec 14, 2021 • edited Loading

sanketmehta28 commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanketmehta28 commented Dec 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikanthccv left a comment

Choose a reason for hiding this comment

sanketmehta28 commented Dec 14, 2021 •

edited

Loading

linux-foundation-easycla bot commented Dec 14, 2021 •

edited

Loading