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

feat(instrumentation/asgi): add target to metrics #1323

Merged
merged 12 commits into from
Oct 22, 2022

Conversation

sk-
Copy link
Contributor

@sk- sk- commented Sep 10, 2022

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 the HTTP_TARGET attribute to the list of metric atttributes, because it will be present but with high cardinality.

Partially fixes #1116

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Besides the included unit tests, the logic was tested using the following app:

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)

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@sk- sk- requested a review from a team September 10, 2022 05:12
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 10, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: sk- / name: Sebastian Kreft (086213c)

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.
@sk-
Copy link
Contributor Author

sk- commented Sep 10, 2022

Not sure what to do regarding the failed lint checks

************* Module opentelemetry.instrumentation.asgi
instrumentation/opentelemetry-instrumentation-asgi/src/opentelemetry/instrumentation/asgi/__init__.py:449:4: R0912: Too many branches (13/12) (too-many-branches)
************* Module tests.test_asgi_middleware
instrumentation/opentelemetry-instrumentation-asgi/tests/test_asgi_middleware.py:1:0: C0302: Too many lines in module (1036/1000) (too-many-lines)
  • How can I test the feature if there are no lines left to be added?
  • One branch could be removed from the method __call__ by changing
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 if target check to that function.

However, I think it'd be best to change collect_request_attributes to return a set of attributes suitable for metrics and another for traces. However, that would require a refactoring several places, as that function is public and actively used today.

sk- and others added 4 commits October 9, 2022 18:09
…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>
@sk-
Copy link
Contributor Author

sk- commented Oct 21, 2022

@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.

@srikanthccv
Copy link
Member

Hm, I am not sure. I don't mind if you add pylint-disable.

@sk-
Copy link
Contributor Author

sk- commented Oct 22, 2022

Hm, I am not sure. I don't mind if you add pylint-disable.

Done, let me know if everything looks good now.

@sk- sk- requested a review from srikanthccv October 22, 2022 12:23
@srikanthccv srikanthccv enabled auto-merge (squash) October 22, 2022 13:25
@srikanthccv srikanthccv merged commit d5369a4 into open-telemetry:main Oct 22, 2022
sk- added a commit to sk-/opentelemetry-python-contrib that referenced this pull request Nov 10, 2022
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.
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
saartochner-lumigo pushed a commit to lumigo-io/opentelemetry-python-contrib that referenced this pull request Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants