Skip to content

Conversation

andrewstuart
Copy link
Contributor

This enables a little bit more flexibility and less boilerplate for registering many Collectors.

  • One-liner MustRegisterAll(coll1, coll2, coll3, coll4)
  • Spread a slice MustRegisterAll(collectors...)

func RegisterAll(m ...Collector) error {
for i := range m {
if err := Register(m[i]); err != nil {
return err
Copy link
Member

Choose a reason for hiding this comment

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

The problem here is that you don't know at which Collector the failure has happen if an error is returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would you think about returning a descriptive error containing the exact argument that failed? Then the usage between the two functions gets really clean, since it's essentially the same as before, but variadic for both.

Copy link
Member

Choose a reason for hiding this comment

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

I think that is way to complex to save the user a single for loop. Also, it makes the error handling way more convoluted.

@beorn7
Copy link
Member

beorn7 commented Jul 11, 2016

Thanks for your contribution.

Ironically, I'm currently working on making the registry interface leaner (to finally expose a public registry interface).

Those variadic functions only save one line of code, essentially. I'd claim it's more idiomatic Go to write an additional line of code instead of bloating the number of functions. In addition, as commented before, in the case of RegisterAll, the behavior if an error is returned somewhere in the middle is problematic.

I suggest to not add a RegisterAll because of the error return problem, and to make MustRegister variadic so that we can even avoid adding MustRegisterAll. Does that make sense?

@andrewstuart
Copy link
Contributor Author

So I've gone ahead and updated MustRegister, though I'd like to propose merging the original RegisterAll and Register, while returning a more-descriptive error about where in the arg list the registration failed.

I think this interface gives the best of both worlds: still only two functions, flexible input, and helpful, descriptive errors. Let me know what you think. If you'd still rather keep the same arity for Register(), I can refactor again; just let me know. :-)

@beorn7
Copy link
Member

beorn7 commented Jul 11, 2016

I would say, leave Register as it is and just have MustRegister variadic (and adjust the doc comment accordingly).

@andrewstuart
Copy link
Contributor Author

Alright, let me know how this looks. I also squashed to keep a cleaner history.

@beorn7
Copy link
Member

beorn7 commented Jul 11, 2016

👍 I like that. Merging...

@beorn7 beorn7 merged commit 28be158 into prometheus:master Jul 11, 2016
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