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

Add linter helpers to testutil #743

Merged
merged 3 commits into from
Apr 27, 2020
Merged

Add linter helpers to testutil #743

merged 3 commits into from
Apr 27, 2020

Conversation

beorn7
Copy link
Member

@beorn7 beorn7 commented Apr 24, 2020

This for one adds direct MetricFamily support to promlint.

It then adds helpers CollectAndLint and GatherAndLint akin to CollectAndCompare and GatherAndCompare to the testutil package, which simplifies to integrate linting into testing of instrumented code.

@RainbowMango

Also, change all the `dto.MetricFamily` arguments to pointers to be
more consistent with what we do in client_golang in general.

Signed-off-by: beorn7 <beorn@grafana.com>
Signed-off-by: beorn7 <beorn@grafana.com>
@beorn7 beorn7 requested review from brancz and bwplotka April 24, 2020 21:47
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Awesome! 🤗

LGTM, some tiny suggestions only.

)

// CollectAndLint registers the provided Collector with a newly created
// pedantic Registry. It then does the same as GatherAndLint, gathering the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// pedantic Registry. It then does the same as GatherAndLint, gathering the
// pedantic Registry. It then does the same as GatherAndLint, gathers the

Kind of scared to suggest this to you, but .... am I right here? 😄

Previous version was reading oddly

Copy link
Member Author

Choose a reason for hiding this comment

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

I could argue why my version is correct. But I agree it's convoluted. I'll reword to avoid the complication.

@@ -29,7 +29,8 @@ import (
// A Linter is a Prometheus metrics linter. It identifies issues with metric
// names, types, and metadata, and reports them to the caller.
type Linter struct {
r io.Reader
r io.Reader
Copy link
Member

@bwplotka bwplotka Apr 25, 2020

Choose a reason for hiding this comment

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

I would comment that this is oneof between two. I was bit confused and I can imagine package devs might be as well..

Not relevant to package users

Copy link
Member Author

Choose a reason for hiding this comment

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

It would actually work with both having values, just that you don't have a constructor doing so (but we might have in the future). I'll add a comment explaining that.

if err != nil {
return nil, fmt.Errorf("gathering metrics failed: %s", err)
}
if metricNames != nil {
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 I would prefer len(metricNames) > 0, less surprising.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking was that the linter will never recognize fewer problems in future versions, but it might recognize more. I changed the comment to clarify that.

Copy link
Member

Choose a reason for hiding this comment

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

Thx!

Signed-off-by: beorn7 <beorn@grafana.com>
@RainbowMango
Copy link
Contributor

Awesome!

@RainbowMango
Copy link
Contributor

@beorn7
By the way, are you gonna publish a new release after this PR?

I can help clean up promlint package in prometheus/prometheus repo.

I was wondering if we can deprecate promlint in prometheus/prometheus.

@beorn7
Copy link
Member Author

beorn7 commented Apr 27, 2020

@RainbowMango Yes, I'll cut v1.6.0 ASAP (hopefully today).

Once it is released, it would be very much appreciated if you could help switching over the prometheus/prometheus code.

@beorn7 beorn7 merged commit 00d5f9e into master Apr 27, 2020
@beorn7 beorn7 deleted the beorn7/test branch April 27, 2020 12:49
@RainbowMango
Copy link
Contributor

Once it is released, it would be very much appreciated if you could help switching over the prometheus/prometheus code.

OK. Will do.

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.

3 participants