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 for ensuring correct Prometheus metric name. #2430

Closed
bwplotka opened this issue Apr 14, 2020 · 30 comments · Fixed by #3992
Closed

Add linter for ensuring correct Prometheus metric name. #2430

bwplotka opened this issue Apr 14, 2020 · 30 comments · Fixed by #3992

Comments

@bwplotka
Copy link
Member

Thanks to @miekg and code they added here: https://github.com/coredns/coredns/blob/master/test/metric_naming_test.go

@sresthas
Copy link

I would like to take this up as my first issue. Any pointers to start would be helpful.
Thanks :-)

@bwplotka
Copy link
Member Author

Well, I know as much as you! I got that awesome link, and we need to have the same on Thanos side. Ideally, it is a separate Go command as we do in copyright check: https://github.com/thanos-io/thanos/blob/55cb8ca38b3539381dc6a781e637df15c694e50a/scripts/copyright/copyright.go

At some point we could propose it as official linter for https://github.com/golangci/golangci-lint 🤗

@yeya24
Copy link
Contributor

yeya24 commented Apr 16, 2020

A command-line linter would be super useful! This also benefits all the projects using Prometheus metrics in the Go ecosystem

@sresthas
Copy link

Should I add it as a test? Or a separate go command?

@bwplotka
Copy link
Member Author

bwplotka commented Apr 17, 2020 via email

@sresthas
Copy link

@bwplotka no I haven't checked that, I was learning go, should I check that first and maybe open an issue there?

@bwplotka
Copy link
Member Author

Just check their docs, I am sure it's easy to add and there must be some process

@sresthas
Copy link

https://github.com/golangci/golangci-lint/wiki/How-to-add-a-custom-linter
I found this, so should I try adding the linter in golint ci?

@bwplotka
Copy link
Member Author

We can try, question is if they will accept Prometheus instrumentation to be popular enough. IMO it is native way for metrics. Let's see (:

In the worst case we can use the linter you will build separatedly to golangci-lint! (:

@pracucci
Copy link
Contributor

@twisted-sres Feel free to add me as reviewer too. I would like to do the same in Cortex too, and we may reuse what you're coming up with.

@mcristina422
Copy link

I would love to help/see this eventually as a customer linter in golangci-lint. I think we're going to run into the same problem that k8s and others ran into with promlint, adding it will add a TON of unrelated dependencies.
prometheus/prometheus#5317

@stale
Copy link

stale bot commented May 20, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label May 20, 2020
@yeya24
Copy link
Contributor

yeya24 commented May 20, 2020

Valid and help wanted!

@stale stale bot removed the stale label May 20, 2020
@stale
Copy link

stale bot commented Jun 19, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jun 19, 2020
@stale
Copy link

stale bot commented Jun 26, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Jun 26, 2020
@GiedriusS
Copy link
Member

Still relevant AFAIK.

@GiedriusS GiedriusS reopened this Jun 30, 2020
@stale stale bot removed the stale label Jun 30, 2020
@kakkoyun
Copy link
Member

Something is baking https://github.com/yeya24/promlinter @yeya24

@yeya24
Copy link
Contributor

yeya24 commented Jul 24, 2020

Opened a pr in golangci-lint, I am not sure whether it will be accepted or not, but it works (though not all the cases).

Screenshot from 2020-07-23 21-26-14

@miekg
Copy link
Contributor

miekg commented Jul 24, 2020 via email

@bwplotka
Copy link
Member Author

@miekg Actually for this one you don't need linter, you ensure that through library itself. Check out promauto.With and discussion here: #2102

Since we moved all metric constructions to this, it's not longer a problem (:

@miekg
Copy link
Contributor

miekg commented Jul 24, 2020 via email

@stale
Copy link

stale bot commented Aug 23, 2020

Hello 👋 Looks like there was no activity on this issue for last 30 days.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity for next week, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Aug 23, 2020
@GiedriusS
Copy link
Member

@yeya24 seems like there is interest from golangci-lint. Care to update your PR there? This would be very much appreciated

@stale stale bot removed the stale label Aug 23, 2020
@yeya24
Copy link
Contributor

yeya24 commented Aug 23, 2020

Thanks @GiedriusS. Yes, I want to finish that as well. But tbh that tool is not very mature and complete currently and it cannot handle some cases. So I am wondering whether it is a good time to add it to golangci-lint or not.

Anyway, I will update that pr soon.

@stale
Copy link

stale bot commented Oct 23, 2020

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 23, 2020
@stale
Copy link

stale bot commented Nov 6, 2020

Closing for now as promised, let us know if you need this to be reopened! 🤗

@stale stale bot closed this as completed Nov 6, 2020
@yeya24
Copy link
Contributor

yeya24 commented Nov 6, 2020

Still valid. Will get back to this soon

@yeya24 yeya24 reopened this Nov 6, 2020
@stale stale bot removed the stale label Nov 6, 2020
@stale
Copy link

stale bot commented Jan 5, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Jan 5, 2021
@yeya24
Copy link
Contributor

yeya24 commented Jan 5, 2021

valid

@stale stale bot removed the stale label Jan 5, 2021
@stale
Copy link

stale bot commented Mar 16, 2021

Hello 👋 Looks like there was no activity on this issue for the last two months.
Do you mind updating us on the status? Is this still reproducible or needed? If yes, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next two weeks, this issue will be closed (we can always reopen an issue if we need!). Alternatively, use remind command if you wish to be reminded at some point in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants