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

Porting promlint from prometheus/prometheus #739

Merged
merged 2 commits into from Apr 24, 2020

Conversation

RainbowMango
Copy link
Contributor

Refer prometheus/prometheus#5317.

I split this work into two commits:

  • Porting : Just simply copy promlint in.
  • Minor Improvement: Fix static check warning by some IDE, such as Goland. More Deatils

I wish it will be easier for the reviewer to catch what has been changed during porting.

Signed-off-by: RainbowMango <renhongcai@huawei.com>
@RainbowMango
Copy link
Contributor Author

RainbowMango commented Apr 23, 2020

cc @beorn7 @brancz

@RainbowMango
Copy link
Contributor Author

RainbowMango commented Apr 23, 2020

I can't see the Travis build log, travis-ci.org always say Under Maintenance when access the Details link. I will check it later.

@brancz
Copy link
Member

brancz commented Apr 23, 2020

lgtm but leaving last call to @beorn7

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.

LGTM 👍

Copy link
Member

@beorn7 beorn7 left a comment

Choose a reason for hiding this comment

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

Just some nits here. I'll ask a more fundamental question separately in the conversation.

@@ -0,0 +1,367 @@
// Copyright 2017 The Prometheus Authors
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't matter a lot, but we use to fill in the current year if we create a new file.

prometheus/testutil/promlint/promlint.go Show resolved Hide resolved
prometheus/testutil/promlint/promlint_test.go Outdated Show resolved Hide resolved
Text string
}

// newProblem is helper function to create a Problem.
Copy link
Member

Choose a reason for hiding this comment

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

🤣

}

// New creates a new Linter that reads an input stream of Prometheus metrics.
// Only the text exposition format is supported.
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "Only the Prometheus text exposition format is supported." so that people don't get confused with OpenMetrics?

@beorn7
Copy link
Member

beorn7 commented Apr 23, 2020

Many thanks 🌈🥭, 🇧🇱🐟, and ☮️.

I have a question: Do you feel this should in its own promlint package? I'm a bit concerned the package structure gets a bit nested. On the other hand, it's a nice and clean separation. If we keep it in a sub-package, I'd like to add testutil.CollectAndLint and testutil.GatherAndLint to make it easier to include the linting in metrics testing. Do you think that's a good idea?

Signed-off-by: RainbowMango <renhongcai@huawei.com>
@RainbowMango
Copy link
Contributor Author

I have a question: Do you feel this should in its own promlint package? I'm a bit concerned the package structure gets a bit nested.

I'm glad you ask this.

Yes, I think if we put it in client_golang/prometheus/promlint would be much better.
But I'm not worried about the nest, I think promlint not just test util. It could be more significant in the future.

Indeed, we can take promlint as test util for now, because it can only check the gathered metrics.
I have a thought to extend promlint, making it possible to check a metric before register to a registry. So, we can take promlint as a tool for metric conformance. From this point of view, even we put promlint to a separated repo could be acceptable.

@RainbowMango
Copy link
Contributor Author

Actually, I started a program to run my thought months ago.

I don't want to disrupt the Prometheus community, I just want the best practice cloud spread to more projects.

Would you mind I continue working on my program? If it really works, I will be happy to donate it to the Prometheus community.

@beorn7
Copy link
Member

beorn7 commented Apr 24, 2020

We already arrived at the conclusion that having the promlint package here in client_golang is a good idea. A stand-alone program would than simply import the package and do the boilerplate around it to turn it into e.g. a command line tool or whatever else.

My question was more about having the moving parts directly in client_golang/prometheus/testutil or in its own sub-package client_golang/prometheus/testutil/promlint as it is now in this PR. I'd find client_golang/prometheus/promlint confusing as I see the linting as part of testing from the perspective of an instrumentation library.

Let's go with client_golang/prometheus/testutil/promlint as in this PR for now. I'll sand a PR for those helpers in testutil I mentioned above.

Thanks everyone and in particular @RainbowMango .

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.

None yet

4 participants