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

Declare registry on MetricWrapperBase as Optional #754

Merged

Conversation

reivilibre
Copy link
Contributor

Dear maintainer @csmarchbanks,

The release of 0.13.0 yesterday appears to have caused us some type checking errors with Mypy (presumably this is a recent effort to annotate this library with types — thanks!).

I think this particular annotation is incomplete though — our code relies on passing None here and the code beneath says the following:

            if registry:
                registry.register(self)

In other words, I think this was intended to be optional.

Thanks!

Signed-off-by: Olivier Wilkinson (reivilibre) [email protected]

Signed-off-by: Olivier Wilkinson (reivilibre) <[email protected]>
Copy link
Member

@csmarchbanks csmarchbanks left a comment

Choose a reason for hiding this comment

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

Thanks and good catch! When I look at your linked PR you are also using a custom proxy registry instead of a CollectorRegistry. Perhaps for now we should just leave out type checking on the registry until a Protocol or abc is created?

Would you also be able to fix the same issue in Enum in this file as well?

Once merged I will cut 0.13.1

@csmarchbanks
Copy link
Member

FYI I am also going to allow Any as labelvalue types as we cast them to str anyway: #755.

Signed-off-by: Olivier Wilkinson (reivilibre) <[email protected]>
@reivilibre reivilibre force-pushed the rei/collector_registry_is_optional branch from 19bc32f to 86ddcac Compare January 28, 2022 14:56
@reivilibre
Copy link
Contributor Author

Perhaps for now we should just leave out type checking on the registry until a Protocol or abc is created?

I think it's fair enough to leave it as is (with the type annotations) — if people like us are so keen on doing dubious things then a cast is OK to work around the problem in the meantime (it's no worse than it being Any — but for people who are respecting the annotations, it provides a little bit more safety net).

FYI I am also going to allow Any as labelvalue types as we cast them to str anyway

Thanks! I noticed they get cast to str internally but it's one of those things where you shouldn't second-guess the type annotations too much (though I suppose at this point, people are relying on the casts being there). It's good to have it stated explicitly, though — thanks for that (and for letting me know).

I've updated the PR to do the same for Enum :).

@csmarchbanks
Copy link
Member

That sounds good, thanks!

@csmarchbanks csmarchbanks merged commit b44b63e into prometheus:master Jan 28, 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