-
Notifications
You must be signed in to change notification settings - Fork 604
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
feat(instrumentation/asgi): add target to metrics #1323
Conversation
|
This PR adds the target information for metrics reported by instrumentation/asgi. Unfortunately, there's no ASGI standard to reliably get this information, and I was only able to get it for FastAPI. I also tried to get the info with Sanic and Starlette (encode/starlette#685), but there's nothing in the scope allowing to recreate the route. Besides the included unit tests, the logic was tested using the following app: ```python import io import fastapi app = fastapi.FastAPI() def dump_scope(scope): b = io.StringIO() print(scope, file=b) return b.getvalue() @app.get("/test/{id}") def test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} sub_app = fastapi.FastAPI() @sub_app.get("/test/{id}") def sub_test(id: str, req: fastapi.Request): print(req.scope) return {"target": _collect_target_attribute(req.scope), "scope": dump_scope(req.scope)} app.mount("/sub", sub_app) ``` Partially fixes open-telemetry#1116 Note to reviewers: I tried to touch as less as possible, so that we don;t require a refactor before this change. However, we could consider changing `collect_request_attributes` so that it returns both a trace attributes and a metrics attributes. Wihout that change we cannot add the `HTTP_TARGET` attribute to the list of metric atttributes, because it will be present but with high cardinality.
Not sure what to do regarding the failed lint checks
for key, value in attributes.items():
current_span.set_attribute(key, value) to current_span.set_attributes(attributes) However, that's not even related to this PR. I could also have a function that modifies the list of attributes, and hence moving the However, I think it'd be best to change |
...tation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py
Outdated
Show resolved
Hide resolved
instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py
Outdated
Show resolved
Hide resolved
…asgi_middleware.py Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
…emetry/instrumentation/asgi/__init__.py Co-authored-by: Srikanth Chekuri <srikanth.chekuri92@gmail.com>
@srikanthccv any advice regarding what to do to fix the lint violations? See #1323 (comment) It's complaining about too many lines in the test file and too many branches in the main function. |
Hm, I am not sure. I don't mind if you add pylint-disable. |
Done, let me know if everything looks good now. |
Unfortunately I made a mistake in open-telemetry#1323 where I assumed that the scope was laready being processed by the framework at the moment of reporting, but of course that's not true as the framework has not even seen the request at that point. Is for that reason that we are not able to extract any route information in the middleware to report what the target is. Sorry for the noise, and be happy to help if anyone has any idea how we could fix this.
Description
This PR adds the target information for metrics reported by instrumentation/asgi.
Unfortunately, there's no ASGI standard to reliably get this information, and I was only able to get it for FastAPI.
I also tried to get the info with Sanic and Starlette (encode/starlette#685), but there's nothing in the scope allowing to recreate the route.
Note to reviewers: I tried to touch as less as possible, so that we don;t require a refactor before this change. However, we could consider changing
collect_request_attributes
so that it returns both a trace attributes and a metrics attributes. Wihout that change we cannot add theHTTP_TARGET
attribute to the list of metric atttributes, because it will be present but with high cardinality.Partially fixes #1116
Type of change
How Has This Been Tested?
Besides the included unit tests, the logic was tested using the following app:
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.