Ensure that nil registers are treat as a no-op, even when wrapping.

[tomwilkie]

Passing a nil is useful in tests, but our treatment of nil is inconsistent.

This code does not panic:

promauto.With(nil).NewCounter(...)

Where as the following does panic.

promauto.With(prometheus.WrapRegistererWith(prometheus.Labels{"foo": "bar"}, nil)).NewCounter(...)

Signed-off-by: Tom Wilkie tom.wilkie@gmail.com

[bwplotka]

TBH I think we might instead of this go for noop pattern instead. If we would have just prometheus.NoopRegistry we could just remove all nil checks and go for it - it would much cleaner nicer.

Also technically those are two separate packages, so no need to be consistent logic wise, but it makes sense to me. 👍

So LGTM, although I would prefer noop pattern - we can changed that later I guess.

Also: I think this might deserve some comment on those method, but probably @beorn7 might know more where and if required, so deffering to him (:

[tomwilkie]

I'm a fan of the NoopRegisterer approach too, but I spoke with @beorn7 and he preferred this approach, and I kinda agree with him now. We already assume nil is safe in promauto, and therefore adding a NoopRegisterer would not allow us to remove these checks, in effect doing it both ways. Plus this way the zero value for Registerer is a valid, which is mode idiomatic, no?

[beorn7]

Could you also add to the doc comment of WrapRegistererWith and WrapRegistererWithPrefix that wrapping a nil value is valid, resulting in a no-op Registerer?

[beorn7]

Registerer is an interface type, so I'm not sure if it is correct to talk about a zero value for it. But I do like to encourage that functions receiving a Registerer will treat a nil value benignly.

It's trivial to implement a no-op implementation of a Registerer if you need it. However, if we provided one in the prometheus package, it would signal that that's the usual way to go. Then we would inevitably have some people implementing functions that want a NoopRegisterer to signal "no instrumentation", while others will implement functions that are also happy with nil. I like the latter because users of that function don't even have to import the prometheus package if the don't want instrumentation.

[beorn7]

Will merge on green!

[beorn7]

TestSummaryDecay has started to become flaky recently. See #765

[bwplotka]

Thanks for this discussion, overall I agree with the way to go for now, but I would be curious what would be the best approach for future (e.g v2)

Then we would inevitably have some people implementing functions that want a NoopRegisterer to signal "no instrumentation", while others will implement functions that are also happy with nil. I like the latter because users of that function don't even have to import the prometheus package if they don't want instrumentation.

I don't get why import is the problem. Anyway, I think I agree with this direction going forward. While I definitely like explicitly marking no instrumentation with Noop, however, I tend to agree that people will just create wrappers that accept nils... Actually we did that too: https://github.com/thanos-io/thanos/blob/55cb8ca38b3539381dc6a781e637df15c694e50a/pkg/extprom/extprom.go#L10