-
Notifications
You must be signed in to change notification settings - Fork 652
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
starlette instrumentation #777
starlette instrumentation #777
Conversation
tox does exact match on fields delimited by a dash. Thus, any instrumentation that includes "instrumentation" in the name would collide with testing of the "opentelemetry-instrumentation" package. Renaming the tox environment resolves the issue.
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 overall, a few nits + questions I'm not sure about. There is some formatting stuff that isn't really relevant to starlette, but it looks good to me so 🤷
ext/opentelemetry-ext-asgi/src/opentelemetry/ext/asgi/__init__.py
Outdated
Show resolved
Hide resolved
ext/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py
Show resolved
Hide resolved
ext/opentelemetry-instrumentation-starlette/tests/test_starlette_instrumentation.py
Outdated
Show resolved
Hide resolved
...ntelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py
Show resolved
Hide resolved
removing remaining asgi references.
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.
LGTM
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 pretty good! Just a couple of nits. The request for changes is on the typo in the tox file.
...ntelemetry-instrumentation-starlette/src/opentelemetry/instrumentation/starlette/__init__.py
Show resolved
Hide resolved
""" | ||
if not getattr(app, "is_instrumented_by_opentelemetry", False): | ||
app.add_middleware( | ||
OpenTelemetryMiddleware, |
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.
Just out of curiosity, are we creating a starlette instrumentation (even though we can use asgi) for the same reason we have a flask instrumentation (when we could have recommended wsgi)?
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.
Kind of. We need a language-specific instrumentation no matter what, to add the "http.route" span attribute (there is no generic way to express route either wsgi or asgi).
Flask has an extended reason: to avoid calling updateName. However, I finally have an OTEP drafted that I believe will eliminate the deferred updateName requirement: open-telemetry/oteps#115. At that point we can extend wsgi to create the flask instrumentation, in a very similar way to how this instrumentation works.
Co-authored-by: alrex <aboten@lightstep.com> Co-authored-by: Leighton Chen <lechen@microsoft.com>
was breaking all instrumentation tests
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 really good, excited to try! |
Addresses part of #710.