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.

Sign up for GitHub

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

Jump to bottom

Use the right type on Response for tests #139

Merged
merged 6 commits into from
Jun 12, 2022
Merged

Use the right type on Response for tests #139

merged 6 commits into from
Jun 12, 2022

Conversation

Kludex
Copy link
Contributor

@Kludex Kludex commented May 26, 2022 •
edited
Loading

Changes

  • Replace starlette.responses.Response by requests.Response on tests, as it's the right type returned by TestClient.
  • Replace middleware implementation from BaseHTTPMiddleware to pure ASGI.

Can you approve the pipeline @trallnag ? cc @tiangolo - Some tests still failing atm.

Kludex
Kludex commented May 26, 2022
View reviewed changes
tests/test_instrumentation.py
@@ -100,7 +101,7 @@ def get_response(client: TestClient, path: str) -> Response:
return response


def assert_is_not_multiprocess(response: Response) -> None:
def assert_is_not_multiprocess(response: TestClientResponse) -> None:
assert response.status_code == 200
assert b"Multiprocess" not in response.content
assert b"# HELP process_cpu_seconds_total" in response.content
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line of code doesn't pass locally... Which is incredibly annoying.

@trallnag
Copy link
Owner

trallnag commented Jun 6, 2022

Hey @Kludex, thanks for your contribution efforts. Generally I'm open towards switching to a pure ASGI implementation. But sadly I can't support you with the implementation because I lack knowledge regarding ASGI and async Python in general. Can you tell me what the advantage of switching to an ASGI implementation?

@trallnag
Copy link
Owner

trallnag commented Jun 6, 2022

Some tests are failing. It seems like something about the way paths / handlers are exposed has changed. For example this test is failing. I think it is due to the instrumentator not passing all parameters to the middleware. Looking at excluded_handlers

@trallnag trallnag mentioned this pull request Jun 11, 2022
Closed
@adriangb
Copy link
Contributor

Can you tell me what the advantage of switching to an ASGI implementation?

BaseHTTPMiddleware (which is what app.middleware uses) is a wrapper around generic ASGI middleware to expose a request/response interface. Internally it is very complex, and has several bugs which we don't think can ever be resolved. Because of this, we discourage its use. You can read more here: https://www.starlette.io/middleware/#basehttpmiddleware

Unrelated this this PR: just FYI I couldn't get tests to run on an M1 Mac, I had to run them in a Docker container.

@codecov
Copy link

codecov bot commented Jun 12, 2022

Codecov Report

Merging #139 (147f394) into master (1032887) will decrease coverage by 0.30%.
The diff coverage is 90.21%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
- Coverage   93.84%   93.53%   -0.31%     
==========================================
  Files           9       10       +1     
  Lines         893      928      +35     
==========================================
+ Hits          838      868      +30     
- Misses         55       60       +5
Flag Coverage Δ
unittests 93.53% <90.21%> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
prometheus_fastapi_instrumentator/middleware.py 88.88% <88.88%> (ø)
...ometheus_fastapi_instrumentator/instrumentation.py 91.22% <100.00%> (+2.65%) ⬆️
tests/test_instrumentation.py 96.36% <100.00%> (+0.02%) ⬆️
tests/test_instrumentator_expose.py 100.00% <100.00%> (ø)
tests/test_metrics.py 97.80% <100.00%> (ø)
prometheus_fastapi_instrumentator/metrics.py 97.52% <0.00%> (-2.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1032887...147f394. Read the comment docs.

@trallnag trallnag merged commit a9e68e3 into trallnag:master Jun 12, 2022
@github-actions
Copy link
Contributor

🎉 This PR is included in version 5.8.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Kludex
Copy link
Contributor Author

Kludex commented Jun 12, 2022

Thanks 👍

@Kludex
Copy link
Contributor Author

Kludex commented Jun 13, 2022

I didn't notice the title was wrong 😅

@Kludex Kludex deleted the replace-middleware-implementation branch June 13, 2022 11:32
@tiangolo
Copy link
Contributor

Niceee! Thanks for this! 🙇

@tiangolo tiangolo mentioned this pull request Jun 13, 2022
Closed
@adriangb adriangb mentioned this pull request Jun 14, 2022
Closed
This was referenced Aug 23, 2022
Merged
Draft
@trallnag trallnag linked an issue Feb 24, 2023 that may be closed by this pull request
Instrumentator Removes Causes from Exceptions #108
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adriangb adriangb adriangb requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Instrumentator Removes Causes from Exceptions
4 participants
@Kludex @trallnag @adriangb @tiangolo