Add CollectAndFormat to testutil, allowing caller to assert as they want to on the exported metric #1503
Conversation
jcass8695
force-pushed
the
main
branch
2 times, most recently
from
f7c7c22
to
695df58
Compare
|
@ArthurSens @bwplotka @kakkoyun if you have time would you mind taking a look at this please? Do you you think it's something that would be useful? |
|
@ArthurSens @bwplotka @kakkoyun bumping this PR. Happy to let it fall into your backlog of reviews, but a 👍🏻 or something to show you've seen it would be much appreciated, thank you 🙏🏻 . |
|
Checking now, sorry for lag! |
prometheus/testutil/testutil.go
Outdated
| // CollectAndFormat registers the provided Collector with a newly created pedantic Registry. It then calls Gather with | ||
| // that Registry and filters by the provided metricNames. It writes the gathered metrics to a bugger formatted according | ||
| // to the provided format argument |
There was a problem hiding this comment.
Makes sense. I will be controversial here and suggest we stop doing so verbose comments. This comment explains implementation. I know you copied that for consistency, but we have to start somewhere. What about focusing on contract and usability. Implementation is trivial and self-described by the code. E.g.:
| // CollectAndFormat registers the provided Collector with a newly created pedantic Registry. It then calls Gather with | |
| // that Registry and filters by the provided metricNames. It writes the gathered metrics to a bugger formatted according | |
| // to the provided format argument | |
| // CollectAndFormat collects the given `metricNames` metrics using pedantic registry and returns in the given format. |
Do you mind fixing GatherAndCompare in the same manner, if that's ok with you?
There was a problem hiding this comment.
Yep, I'm OK with that
prometheus/testutil/testutil.go
Outdated
| // CollectAndFormat registers the provided Collector with a newly created pedantic Registry. It then calls Gather with | ||
| // that Registry and filters by the provided metricNames. It writes the gathered metrics to a bugger formatted according | ||
| // to the provided format argument | ||
| func CollectAndFormat(c prometheus.Collector, format expfmt.FormatType, metricNames ...string) (*bytes.Buffer, error) { |
There was a problem hiding this comment.
Any reason to NOT return bytes or string instead? I would vote for string, it does not matter much. Buffer is weird, usually when you want things to be in buffer, we would need to pass io.Writer in args or something.
There was a problem hiding this comment.
Nope, no reason I don't think. Although I don't totally understand this:
we would need to pass io.Writer in args or something
Why is this the case?
There was a problem hiding this comment.
Perhaps it should be bytes, as the requested format can be protobuf?
CollectAndFormat is a helper function that returns the formatted metrics to the caller, allowing them to use it how they want. This is different to CollectAndCompare where the comparison is done strictly on behalf of the caller. Often it is more convenient to perform a simple substring match on the formatted metric. Signed-off-by: Jack Cassidy <j.cassidy45@gmail.com>
CollectAndFormat is a helper function that returns the formatted metrics to the caller, allowing them to use it how they want. This is different to CollectAndCompare where the comparison is done strictly on behalf of the caller. Often it is more convenient to perform a simple substring match on the formatted metric rather than the full export. This is especially true testing a duration-like gauge metric, where the exact value of the metric might not be easily determined up front.