Conditionally create server spans for flask #828
Conversation
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
...tion/opentelemetry-instrumentation-flask/src/opentelemetry/instrumentation/flask/__init__.py
Outdated
Show resolved
Hide resolved
| if trace.get_current_span() is trace.INVALID_SPAN: | ||
| ctx = extract(flask_request_environ, getter=otel_wsgi.wsgi_getter) | ||
| token = context.attach(ctx) | ||
| span_kind = trace.SpanKind.SERVER | ||
|
|
There was a problem hiding this comment.
If span is found in current context, I believe you'll have to set the current context as the parent. Currently, ctx would be None.
There was a problem hiding this comment.
Done.
I think it was passing earlier as tracer.start_span() makes current span as parent if context passed is none.
@lzchen Should we set parent explicitly for readability or let start_span() handle it?
There was a problem hiding this comment.
Oh yes you are correct. Either way is fine with me.
| @@ -59,7 +59,7 @@ | |||
| if DJANGO_2_0: | |||
| from django.urls import re_path | |||
| else: | |||
| from django.conf.urls import url as re_path | |||
| from django.conf.urls import url as re_path # pylint: disable=E0611 | |||
There was a problem hiding this comment.
Added this as django.conf.urls.url() is removed in django 4.0. leading to lint check failure (release notes).
cc @lzchen
|
The title says flask but contains code for Django. |
Added them to pass lint checks. Just saw those changes have already been merged to master. Removed the django related code. |
* Making span as internal in presence of a span in current context * Updating changelog * Removing extra print statements * Resolving comments: Setting current context as parent in its presence * Ignoring pylint check as django.conf.urls.url is removed in django 4.0 Django release notes: https://docs.djangoproject.com/en/4.0/releases/4.0/ * Removing changes in django files
* Making span as internal in presence of a span in current context * Updating changelog * Removing extra print statements * Resolving comments: Setting current context as parent in its presence * Ignoring pylint check as django.conf.urls.url is removed in django 4.0 Django release notes: https://docs.djangoproject.com/en/4.0/releases/4.0/ * Removing changes in django files
Description
Adds support to make spans as INTERNAL if a span is already present in current context in flask.
Fixes #446
Type of change
Please delete options that are not relevant.
Currently flask only makes SERVER spans. With this PR it can make INTERNAL spans in presence of a span in current context.
How Has This Been Tested?
Added unit test for the changes.
Manually tested the scenario.
To reproduce the issue: Add two instrumentors i.e. FlaskInstrumentor and OpenTelemetryMiddleware in the same app. This creates two separate traces instead of one.
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.