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

Register the inflight_finalizes metric #6724

Merged
merged 1 commit into from Mar 8, 2023
Merged

Register the inflight_finalizes metric #6724

merged 1 commit into from Mar 8, 2023

Conversation

aarongable
Copy link
Contributor

No description provided.

@aarongable aarongable requested a review from a team as a code owner March 7, 2023 19:59
@aarongable
Copy link
Contributor Author

We seriously need a way to guarantee that this bug never happens again; it has bitten us at least three times in the last year.

@pgporada
Copy link
Member

pgporada commented Mar 7, 2023

There's some interesting discussion regarding ensuring all new metrics get registered over here (for a custom registry at least, but still some ideas I guess).

@aarongable
Copy link
Contributor Author

That thread actually leads to a potential solution here: instead of having statsAndLogging return a prometheus.Registry, it could return a promauto.Factory, which exposes all of the usual NewCounterVec etc methods, except that they auto-register the metric when it's created.

@beautifulentropy
Copy link
Member

That thread actually leads to a potential solution here: instead of having statsAndLogging return a prometheus.Registry, it could return a promauto.Factory, which exposes all of the usual NewCounterVec etc methods, except that they auto-register the metric when it's created.

I like this. Lexical confinement == best confinement.

@aarongable
Copy link
Contributor Author

Unfortunately I did some digging and it would be pretty hard for us to use promauto.Factory, because we do lots of stuff with the registries that we pass around. Like, we don't only register individual metrics on them, we also register whole collectors (e.g. the whole grpc-go auto collector), and promauto.Factory doesn't expose the underlying registry at all. So we'd have to either completely overhaul when/where we register various third-party collectors with the registry, or we'd have to be passing around both the registerer and the factory, and that seems unfortunate.

Anyway, it's certainly worth digging into further, but I didn't see a nice clean improvement in about an hour of digging :(

@aarongable aarongable merged commit c55ab19 into main Mar 8, 2023
@aarongable aarongable deleted the reg-metric branch March 8, 2023 21:53
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.

None yet

3 participants