Conditionally create server spans for falcon #867
Conversation
| @@ -195,12 +195,23 @@ def __call__(self, env, start_response): | |||
|
|
|||
| start_time = _time_ns() | |||
|
|
|||
| token = context.attach(extract(env, getter=otel_wsgi.wsgi_getter)) | |||
| token = ctx = span_kind = None | |||
|
|
|||
There was a problem hiding this comment.
this snippet is becoming pretty common. probably could take it out as a re-usable function.
There was a problem hiding this comment.
Added re-usable function to get the three variables. Could not think of better name. Please suggest.
| @@ -195,12 +195,17 @@ def __call__(self, env, start_response): | |||
|
|
|||
| start_time = _time_ns() | |||
|
|
|||
| token = context.attach(extract(env, getter=otel_wsgi.wsgi_getter)) | |||
| token, ctx, span_kind = get_token_context_span_kind( | |||
There was a problem hiding this comment.
I was thinking more like a wrapper function for start_span which internally takes care of checking server spans something like
span = _start_internal_or_server_span(name, start_time, context_getter)this function would accept whatever is necessary to figure out if an existing span is present or not and decide internally whether to create a server span or an internal span, and which context to use as parent.
There was a problem hiding this comment.
This function will need to return token also along with span.
start_internal_or_server_span(tracer, name, start_time, env, context_getter):
..
create internal/server span
..
return span, token
I am not sure if this is okay.
thinking of putting this function in instrumentation/utils.py
Also, we can shorten the recurring snippet if we want to skip the function
as the else part in the snippet is not necessary. Something like this:
token = context = None
span_kind = SpanKind.INTERNAL
if get_current_span() is INVALID_SPAN:
context = extract(carrier, getter=carrier_getter)
token = attach(context)
span_kind = SpanKind.SERVER
Please share your thoughts.
There was a problem hiding this comment.
This function will need to return token also along with span.
That sounds OK. It can return the span + token and let the caller store a reference to the token and detach it at a later time.
There was a problem hiding this comment.
Also, we can shorten the recurring snippet if we want to skip the function
The main issue is that this is a very specific piece of logic strictly followed by all server instrumentation so I think it makes sense to encapsulate the logic in a re-usable function that is easily discoverable and changeable in a single place. Number of lines of code is not a big deal.
There was a problem hiding this comment.
Makes sense
Created wrapper for start_span.
| @@ -72,27 +72,40 @@ def unwrap(obj, attr: str): | |||
| setattr(obj, attr, func.__wrapped__) | |||
|
|
|||
|
|
|||
| def get_token_context_span_kind(env, getter): | |||
| """Based on presence of active span, extracts context and initializes token and span_kind | |||
| def start_internal_or_server_span( | |||
There was a problem hiding this comment.
should be private function so prefix with _
| def get_token_context_span_kind(env, getter): | ||
| """Based on presence of active span, extracts context and initializes token and span_kind | ||
| def start_internal_or_server_span( | ||
| tracer, span_name, start_time, env, context_getter |
There was a problem hiding this comment.
env should be called context
There was a problem hiding this comment.
changed to context_carrier as context is already imported module and context is extracted from env object.
Let me know if you want me to rename it.
|
Thanks @ashu658! Would be great to update other implementations to use the same function. |
* Making span as internal for falcon in presence of a span in current context * Updating changelog * Fixing lint and generate build failures * Resolving comments: Converting snippet to re-usable function * Fixing build failures * Resolving comments: Creating wrapper for start span to make internal/server span * Rerun docker tests * Resolving comments: Refactoring
* code change to resolve the bug #449 * modifying the changelog file to add entry for PR #869 * removing redundent get statement * Conditionally create server spans for falcon (#867) * Making span as internal for falcon in presence of a span in current context * Updating changelog * Fixing lint and generate build failures * Resolving comments: Converting snippet to re-usable function * Fixing build failures * Resolving comments: Creating wrapper for start span to make internal/server span * Rerun docker tests * Resolving comments: Refactoring * Fix Django 1.9 issue preventing use of MIDDLEWARE_CLASSES (#870) * Update CHANGELOG.md * Fix Django 1.9 issue preventing use of MIDDLEWARE_CLASSES Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> * changing the import trace statement to resolve issue with unit test cases Co-authored-by: Ashutosh Goel <39601429+ashu658@users.noreply.github.com> Co-authored-by: Dan <pezzer55@gmail.com> Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com> Co-authored-by: Owais Lone <owais@users.noreply.github.com>
Description
Adds support to make spans as INTERNAL if a span is already present in current context in falcon.
Fixes #447
Type of change
Please delete options that are not relevant.
Currently falcon 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. FalconInstrumentor 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.