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

Client library guidelines: Cannot enforce global state #506

Open
beorn7 opened this issue Aug 3, 2016 · 7 comments
Open

Client library guidelines: Cannot enforce global state #506

beorn7 opened this issue Aug 3, 2016 · 7 comments

Comments

@beorn7
Copy link
Member

beorn7 commented Aug 3, 2016

The client library guidelines contain the following sentence: "There MUST be a default CollectorRegistry, the standard metrics MUST by default implicitly register into it with no special work required by the user."

In most languages, that means to keep some global state or a singleton around. In other languages, it might be plainly impossible.

The real problem arises for languages that allow global state but it is highly frowned upon. Such a case is Go. The resistance against global state is very high (certainly higher than the resistance against dependencies, to draw a comparison, with the difference that it's mostly the whiners that are against dependencies, while the resistance against global state is lead by the most enlightened members of the community). While I find myself more in the "no global state" camp, I'm also a fan of giving options to the users, if need be let them chose their own poison. (One argument here is that they would anyway create wrappers to have global state again.) So the go client library allows both, work with the problematic global state and earn some convenience in exchange, or avoid the global state altogether. The library doesn't inflict any undue overhead on either camp. However, if auto-registration were the default, there would be special work required by the user to prevent interaction with the global state. That's a big no-no.

And then there is a completely different problem with Go: error handling. Registration can result in an error. The convenient way to create collector instances is in variable initialization. However in the var section, there cannot be any code to handle the error. The usual way out is MustRegister, i.e. panic in case of an error. That's fine for most. However, panicking in the var section is quite problematic (you won't even see one of your functions in the call stack), in particular because an unrelated change somewhere else (where another, but incompatible collector is registered) can suddenly cause a panic in the code that was fine so far. Panicking in this way is, as said, problematic, but doing so as default behavior, without calling it out (with the Must... idiom), is another big no-no.

We could go with a "MustRegister" method on the Collectors or a "MustRegisterWith" option, but even that would probably look weird for most experienced Go programmers, and it could still not do the registration by-default. (The default had to be to not register.)

Whatever the decision for the Go client will be, we have to soften the language in the quote above.

@brian-brazil
Copy link
Contributor

The real problem arises for languages that allow global state but it is highly frowned upon.

That's nothing to do with specific languages, this is an opinion that crosses languages. I've seen it expressed across pretty much all the client libraries there are. Go is not special in this regard.

I'm also a fan of giving options to the users, if need be let them chose their own poison.

That I'm fine with that at the client library level.

However, if auto-registration were the default, there would be special work required by the user to prevent interaction with the global state. That's a big no-no.

I think that's perfectly fine. It should be possible, but the default is still to auto-register. That's how it is in Python for example. Java has no particular default for technical reasons, but all the examples point you the right way.

Whatever the decision for the Go client will be, we have to soften the language in the quote above.

It sounds like you're going to end up going the same way as Java, with MustRegister equivalent to Java's Register.

@beorn7
Copy link
Member Author

beorn7 commented Aug 4, 2016

However, if auto-registration were the default, there would be special work required by the user to prevent interaction with the global state. That's a big no-no.

I think that's perfectly fine. It should be possible, but the default is still to auto-register. That's how it is in Python for example. Java has no particular default for technical reasons, but all the examples point you the right way.

I don't understand what you try to accomplish with this statement. Are you saying my analysis of what's considered acceptable in the Go community is wrong? Or are you trying to explain to the Go community why they are wrong?

In the former case: I'm very confident my analysis is spot on. You need to present contrary evidence to convince me otherwise.

In the latter case: I don't think a comment in a Prometheus doc GH issue will convince the Go community. Realistically, I don't think there is any way you can convince the Go community otherwise. (Personally, I don't even think you should, because IMHO the Go community is right.)

The MustRegister method on each of the usual Collector implementation would be a way that would at least be not in direct contradiction with two adamant conventions. It would still be considered bad style. I have to vet that more carefully.

In any case, the wording in the client libraries still has to be changed. "[…] the standard metrics MUST by default implicitly register into it with no special work required by the user" – this would technically be possible in Go, but we are not going to implement it. (Even if we implement the Collector.MustRegister method, it is special work by the user, and the registration is not implicit, but explicit.) Thus, we would be in direct contradiction with a MUST clause in our own client guidelines.

@brian-brazil
Copy link
Contributor

Are you saying my analysis of what's considered acceptable in the Go community is wrong? Or are you trying to explain to the Go community why they are wrong?

I'm saying that what you propose sounds reasonable all things considered, and at least one other client does something similar.

@beorn7
Copy link
Member Author

beorn7 commented Aug 4, 2016

OK, got it. When you later came back to auto-register as the default, I read the rest as supporting that idea.

@brian-brazil
Copy link
Contributor

This appears to have been settled.

@beorn7
Copy link
Member Author

beorn7 commented Apr 7, 2017

Hmm. We still have the sentence "There MUST be a default CollectorRegistry, the standard metrics MUST by default implicitly register into it with no special work required by the user." in the guidelines. While I don't think we'll kick out the global default registry from client_golang, I'm more convinced than ever that we don't want an implicit default registration with the global default registry in client_golang.

@beorn7 beorn7 reopened this Sep 14, 2017
@beorn7
Copy link
Member Author

beorn7 commented Sep 17, 2018

BTW: The Go library now has the promauto package for an easy but explicit opt-in into auto-registration.

Still it doesn't really fit the "by default" part, and the flagged sentence ("There MUST be a default CollectorRegistry, the standard metrics MUST by default implicitly register into it with no special work required by the user.") is still something that cannot or should not be implemented in that way in many languages.

aylei pushed a commit to aylei/docs that referenced this issue Oct 28, 2019
* releases, readme: add the release notes for 2.0.4

* Update 2.0.4
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

No branches or pull requests

2 participants