Improve CollectAndCount

[beorn7]

Now that we have also added CollectAndLint and GatherAndLint, I
thought we should bring CollectAndCount in line. So:

• Add GatherAndCount.
• Add filtering by metric name.
• Add tests.

Minor wart: CollectAndCount should now return (int, error), but that
would be a breaking change as the current version just returns
int. I decided to let the new version panic when it should return an
error. An error is anyway very unlikely, so the biggest annoyance here
is really just the inconsistency.

@bwplotka you might like this.

@cstyan you needed this recently (but as already discussed, with promauto you won't, but your request still inspired this PR)

[cstyan]

I don't think this does what I was thinking about trying to do in a test the other week, but LGTM regardless.

[beorn7]

You wanted to write a test if a certain metric shows up in your exposition. That's possible with the new tooling here because you can just put that metric name into the filter and count if there is at least one in the exposition.

[cstyan]

Isn't the pedantic registry kind of messing with that? I wanted to check if a list of metric names had been registered with an existing prometheus.Registry.

[beorn7]

That's where the GatherAndCount comes to your rescue (also added in this PR).

[bwplotka]

Thanks @beorn7, but generally I don't see the real purpose of this. 🤔

We had an elegant function that was allowing us to just do testutil.Equals(t, 1, CollecAndCount(myMetricVar)) and now we want to make it more verbose and return some error/panic? 🤔

This is quite tragic as we use it in many places and it converts (some day in new version) to:

count, err := CollecAndCount(myMetricVar)
testutil.Ok(t, err)
testutil.Equals(t, 1, count)

This is clearly unnecessary verbose, and this function was done ONLY for testing.

..Anyone asked for that? 🤔 😄

That's why I am not a fan of this change... especially what's the improvement of this if the previous version was working just fine without error, allowing nice, not obtrusive use in test code. Also adding metricName if we have collector passed already does not make any sense to me.. So we should use metric name or collector... or both?

So first question

Can we NOT change CollectAndCount signature at all? And NOT use registering inside (because why?).

Secondly

TBH I would totally NOT recommend using GatherAndCount at all, because the metric name can change anytime plus you can make an easy typo... I liked CollectAndCount because I could do type-safe validation (!). If you can use directly the metric defined in the code.. there's nothing better than that. And getting metric name from metric variable is non-trivial.

So... can we just NOT introduce GatherAndCount? 😄 Is there any user of that? 🤔
Sounds like @cstyan already found a solution.

Alternatives

Since we have Must register hidden in promauto.With anyway. I might think panic is ok...? Or MustCollectAndCount ? But no one even asked for returning explicit error so it also feels weird that we add code that no one asks for.. 🤔 Also I could not see anyone asking for GatherAndCount either...

[beorn7]

@bwplotka You must have misunderstood. CollectAndCount will continue to work in exactly the same way as before. Just that it has now optional filtering.

And as I tried to explain, we don't need to change it to return an error. This PR does not change it to return an error.

The registration is needed to enable the filtering. Since the registration will never fail in practice, I wouldn't introduce a different code path for the case of no filtering.

And yes, there is a need for GatherAndCount. @cstyan did not find another solution, he just did something different that happened to not test for presence of metrics anymore.

And there are always people who ask for the option to handle an error instead of panicking. That's why all the other functions return an error. But as you personally do not seem to need GatherAndCount, there is no reason for you to dislike this because CollectAndCount still does not return an error.

[bwplotka]

CollectAndCount will continue to work in exactly the same way as before. Just that it has now optional filtering.

Hm but the single collector has just one metric name, so what is this filtering 🤔 I guess historgrams / summaries which have more than one? Ok, then fine (:

And as I tried to explain, we don't need to change it to return an error. This PR does not change it to return an error.

I know @beorn7 but you say it's broken that it does not return error... so I am arguing with that. (:

[bwplotka]

And there are always people who ask for the option to handle an error instead of panicking. That's why all the other functions return an error. But as you personally do not seem to need GatherAndCount, there is no reason for you to dislike this because CollectAndCount still does not return an error.

Cool, looks like what we need is Must... version then. (:

[bwplotka]

How brutal would be to introduce breaking change? and mention this in CHANGELOG?

This is particularly new function.

[beorn7]

Hm but the single collector has just one metric name, so what is this filtering?

That's not true. A Collector can return any number of differently named metrics. A prominent example used by almost everyone is https://github.com/prometheus/client_golang/blob/master/prometheus/go_collector.go .

With the filtering, you can easily test for presence of a particular metric in your home-grown Collector implementation.

I guess historgrams / summaries which have more than one?

That's not true, either. (o: Histograms and summaries only have one metric name. (The "magic" metric names you see in Prometheus and in the text format are not "real" names.)

I know @beorn7 but you say it's broken that it does not return error... so I am arguing with that. (:

I'm not saying it's broken. It's just inconsistent with the four or five other functions we have in this package. I'd prefer to have them all panic or all return an error. But either change would be breaking.

Cool, looks like what we need is Must... version then. (:

Strictly speaking, yes, but I think in this case, it would be overkill. Personally, I'd be fine with all the functions panicking, but the error-return version is less controversial and more flexible (even if it makes the handling less elegant – but why are you using Go if you want elegant code? 🤯 ). We got the version with errors from the 1st time contributor, and when you contributed CollectAndCount, I didn't realize that we could align it better with the existing functions.

What we have in this PR now has just a tiny consistency wart.

How brutal would be to introduce breaking change?

Given the gigantic adoption of this module, I'd really bump the major version for any breaking change in a part that is not explicitly marked as experimental. But then I want to bundle a bunch of batched up breaking changes to not inflict the cost of a major version bump to change a single function signature.

[cstyan]

Hmm okay, yes I can see how GatherAndCount could be used for what I wanted to try. Thanks @beorn7 !

[bwplotka]

Thanks for the elaboration. I was wrong with collector behavior then. Filtering by metric name makes. (:

I'm not saying it's broken. It's just inconsistent with the four or five other functions we have in this package. I'd prefer to have them all panic or all return an error. But either change would be breaking.

Ok think I can agree with your friendliness towards users (: so let's not break compatibility.

But still, I don't fully agree with the comment that is "predicting" that panicking is wrong here (:
Sorry for begin pain (: Unless I am missing something, the only kind of errors that can happen in Collector is someone creating an invalid metric name 🤔 Plus this is only testing package.

But I think I get that it would be just easier for library and more solid to return error explicitly...

Damn, we will need to create a test helper, that's it, 🤷‍♂️

LGTM (: Thanks for the discussion, makes sense.

[beorn7]

But still, I don't fully agree with the comment that is "predicting" that panicking is wrong here (:

There will certainly be a few people patronizing me at the next meetup (as it happened before, and of course only if we will have proper meetups again any time soon…). 🤓

Unless I am missing something, the only kind of errors that can happen in Collector is someone creating an invalid metric name

Which can plausibly happen given that the helper function will often be used to test custom collectors. The collector could also collect a set of metrics that is not self-consistent.

Plus this is only testing package.

Some people feel especially strong about not panicking in test code.

In general, instead of trying to convince everyone to do the one right thing, I try to accommodate both schools of thought, at least if that's possible without a lot of overhead.

[bwplotka]

Some people feel especially strong about not panicking in test code.

Kind of I am. But I am sometimes also not decided xd

👍