-
Notifications
You must be signed in to change notification settings - Fork 599
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
Fix ASGI instrumentation default span name #418
Fix ASGI instrumentation default span name #418
Conversation
9bf7bab
to
e34297e
Compare
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Show resolved
Hide resolved
e34297e
to
5c42432
Compare
@@ -205,7 +206,7 @@ async def __call__(self, scope, receive, send): | |||
@wraps(receive) | |||
async def wrapped_receive(): | |||
with self.tracer.start_as_current_span( | |||
span_name + " asgi." + scope["type"] + ".receive" | |||
span_name + " " + scope["type"] + ".receive" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scope["type"].receive
is not defined in the specs. I am wondering what we should do with these spans.
@codeboten thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im wondering if the SpanKind should be set here in addition to having the name of the span reflect the sender/receiver as per the messaging semantic conventions: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/messaging.md#span-name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamantike
According to the specs, we should use a space instead of a period to separate destination name and operation name.
@codeboten
I think we actually should do that, following the specs, however might be out of scope for this pr.
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
@adamantike |
681f456
to
4a7082a
Compare
@lzchen, sorry for the delay, I was waiting for a follow-up regarding the comment on I have rebased, and applied your other comment. |
@adamantike apologies for the delay, i completely missed this in my notifications. |
@adamantike have you had a chance to look at @lzchen's comment? |
02bddf0
to
1880d52
Compare
@lzchen @codeboten, I have replaced the separator for destination and operation names. Please, let me know if that's what we expect based on the specs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Please rebase.
Fixes ASGI default span name for SERVER spans, to be `HTTP <method>`. Fixes open-telemetry#146.
1880d52
to
c977e2f
Compare
Rebased and fixed conflicts! |
Please fix the lint issues, then we can merge this :) |
Can it be that the CI error on the
|
Ahh I see, should be fixed after this. |
Description
Fixes ASGI default span name for SERVER spans, to be
HTTP <method>
.Fixes #146.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Updated unit tests, executed with
tox -e test-instrumentation-asgi
.Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.