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

Test test_expose_defaults fails due to NameError after updating #219

Closed
trallnag opened this issue Feb 26, 2023 · 3 comments · Fixed by #220
Closed

Test test_expose_defaults fails due to NameError after updating #219

trallnag opened this issue Feb 26, 2023 · 3 comments · Fixed by #220

Comments

@trallnag
Copy link
Owner

Pull request #153 introduced new parameter registry to set Prometheus client registry. It also added two tests to cover this enhancement.

After running poetry update on commit 8cec83b the test_expose_defaults started to fail.

info = <prometheus_fastapi_instrumentator.metrics.Info object at 0x7f232b7433d0>

    def instrumentation(info: Info) -> None:
>       TOTAL.labels(info.method, info.modified_status, info.modified_handler).inc()
E       NameError: free variable 'TOTAL' referenced before assignment in enclosing scope

src/prometheus_fastapi_instrumentator/metrics.py:628: NameError

Before that it worked fine. It tried to debug it. The Prometheus client lib is complaining about duplicate metrics as excepted.

Duplicated timeseries in CollectorRegistry: {'http_requests_total', 'http_requests_created', 'http_requests'}

Problems start when updating starlette and fastapi

  • Updating starlette (0.22.0 -> 0.25.0)
  • Updating fastapi (0.88.0 -> 0.92.0)
@trallnag trallnag changed the title Test test_expose_defaults fails due to NameError after updating fastapi and starlette Test test_expose_defaults fails due to NameError after updating Feb 26, 2023
@trallnag
Copy link
Owner Author

Hey @tiangolo, any idea what might have changed with the update that results in the test suddenly failing?

The hotfix seems to work (see PR) but would still be interesting to know

@frankie567
Copy link

Hey there 👋

Came across this and I thought I would put my two cents in. I think this problem was introduced because since Starlette 0.24.0, the middleware stack is built only once when app starts (and not each time add_middleware is called like before). Here is the relevant PR: encode/starlette#2017

IMHO, the fact that test_expose_defaults was passing was an happy consequence of side effects produced by the rebuild of the middleware stack. It shouldn't have passed like this, with the risk of having local variables undefined.

@dhodun
Copy link

dhodun commented Mar 8, 2023

This latest PR 220 fixed the issue in our environment. Looking forward to the PyPI update.
https://github.com/trallnag/prometheus-fastapi-instrumentator/pull/220/files

Was seeing it on an older version of starlette though:

starlette==0.20.4
fastapi==0.85.1
prometheus-fastapi-instrumentator==5.10.0

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 a pull request may close this issue.

3 participants