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

Conditionally create server spans for falcon #867

Merged
merged 8 commits into from
Jan 21, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#817](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/817))
- `opentelemetry-instrumentation-kafka-python` added kafka-python module instrumentation.
([#814](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/814))

- `opentelemetry-instrumentation-falcon` Falcon: Conditionally create SERVER spans
([#867](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/867))
### Fixed

- `opentelemetry-instrumentation-django` Django: Conditionally create SERVER spans
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ def response_hook(span, req, resp):
)
from opentelemetry.instrumentation.utils import (
extract_attributes_from_object,
get_token_context_span_kind,
http_status_to_status_code,
)
from opentelemetry.propagate import extract
from opentelemetry.semconv.trace import SpanAttributes
from opentelemetry.trace.status import Status
from opentelemetry.util._time import _time_ns
Expand Down Expand Up @@ -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(
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 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense
Created wrapper for start_span.

env, getter=otel_wsgi.wsgi_getter
)

span = self._tracer.start_span(
otel_wsgi.get_default_span_name(env),
kind=trace.SpanKind.SERVER,
context=ctx,
kind=span_kind,
start_time=start_time,
)

if span.is_recording():
attributes = otel_wsgi.collect_request_attributes(env)
for key, value in attributes.items():
Expand All @@ -216,7 +221,8 @@ def _start_response(status, response_headers, *args, **kwargs):
status, response_headers, *args, **kwargs
)
activation.__exit__(None, None, None)
context.detach(token)
if token is not None:
context.detach(token)
return response

try:
Expand All @@ -227,7 +233,8 @@ def _start_response(status, response_headers, *args, **kwargs):
exc,
getattr(exc, "__traceback__", None),
)
context.detach(token)
if token is not None:
context.detach(token)
raise


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from falcon import testing

from opentelemetry import trace
from opentelemetry.instrumentation.falcon import FalconInstrumentor
from opentelemetry.instrumentation.propagators import (
TraceResponsePropagator,
Expand Down Expand Up @@ -264,3 +265,18 @@ def test_hooks(self):
self.assertEqual(
span.attributes["request_hook_attr"], "value from hook"
)


class TestFalconInstrumentationWrappedWithOtherFramework(TestFalconBase):
def test_mark_span_internal_in_presence_of_span_from_other_framework(self):
tracer = trace.get_tracer(__name__)
with tracer.start_as_current_span(
"test", kind=trace.SpanKind.SERVER
) as parent_span:
self.client().simulate_request(method="GET", path="/hello")
span = self.memory_exporter.get_finished_spans()[0]
assert span.status.is_ok
self.assertEqual(trace.SpanKind.INTERNAL, span.kind)
self.assertEqual(
span.parent.span_id, parent_span.get_span_context().span_id
)
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,12 @@

from wrapt import ObjectProxy

from opentelemetry import context, trace

# pylint: disable=unused-import
# pylint: disable=E0611
from opentelemetry.context import _SUPPRESS_INSTRUMENTATION_KEY # noqa: F401
from opentelemetry.propagate import extract
from opentelemetry.trace import StatusCode


Expand Down Expand Up @@ -67,3 +70,29 @@ def unwrap(obj, attr: str):
func = getattr(obj, attr, None)
if func and isinstance(func, ObjectProxy) and hasattr(func, "__wrapped__"):
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

Args:
env : object which contains values that are
used to construct a Context. This object
must be paired with an appropriate getter
which understands how to extract a value from it.
getter : an object which contains a get function that can retrieve zero
or more values from the carrier and a keys function that can get all the keys
from carrier.
"""

token = ctx = span_kind = None

if trace.get_current_span() is trace.INVALID_SPAN:
ctx = extract(env, getter=getter)
token = context.attach(ctx)
span_kind = trace.SpanKind.SERVER
else:
ctx = context.get_current()
span_kind = trace.SpanKind.INTERNAL

return token, ctx, span_kind