-
Notifications
You must be signed in to change notification settings - Fork 342
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 constLabels support via cli arg/env var #77
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Have u tried what happens if you use NewNginxPlusCollector
with constLabels=nil
? I think nothing, but just in case. Most of the time, users won't need to use these labels (or they will configure them in prometheus.yaml), this is for a different use case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dean-Coakley thx. Please see my comments and suggestions. Also, don't forget to update the docs.
I wasn't able to configure the const lables using command line argument :( this might be my mistake though
@Rulox Yeah. Nothing happens if |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dean-Coakley thanks! looks good. Found a few new small problems.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dean-Coakley the latest changes lgtm!
During testing I discovered how the exporter handles invalid metrics labels:
./nginx-prometheus-exporter -nginx.scrape-uri=http://demo.nginx.com/stub_status -prometheus.const-labels="a=b,c a=d"
2019/12/09 13:02:24 Starting NGINX Prometheus Exporter Version=0.4.2 GitCommit=8e22a75
panic: descriptor Desc{fqName: "nginxexporter_build_info", help: "Exporter build information", constLabels: {}, variableLabels: []} is invalid: "c a" is not a valid label name for metric "nginxexporter_build_info"
goroutine 1 [running]:
github.com/prometheus/client_golang/prometheus.(*Registry).MustRegister(0xc0000aa3c0, 0xc0000f3d78, 0x1, 0x1)
/Users/michael/work/nginx/prometheus/go/src/github.com/nginxinc/nginx-prometheus-exporter/vendor/github.com/prometheus/client_golang/prometheus/registry.go:391 +0xad
main.main()
/Users/michael/work/nginx/prometheus/go/src/github.com/nginxinc/nginx-prometheus-exporter/exporter.go:279 +0x464
The message doesn't look user friendly. I suggest we try to fix it. I'm ok if we do that in a separate PR.
Let me know your thoughts @Dean-Coakley @Rulox
@pleshakov I will improve validation in this PR. |
Proposed changes
Adds support for specifying a list of constant labels that all the metrics of the exporter will have.
prometheus.const-labels
CONST_LABELS
Checklist
Before creating a PR, run through this checklist and mark each as complete.