-
Notifications
You must be signed in to change notification settings - Fork 81
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
Basic auth: add metrics #24
base: master
Are you sure you want to change the base?
Conversation
e1d4cf1
to
4fc67c9
Compare
Ready for a second review, once that is in I will add metrics for the TLS part (or I can do in the same PR if you want) |
u.failuresCounter = prometheus.NewCounter( | ||
prometheus.CounterOpts{ | ||
Namespace: "prometheus_toolkit", | ||
Subsystem: "https", |
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.
https and basic auth are not the same thing
In general, it's better to have the metric name as a single string literal as it makes grepping and determining what the actual metric name is easier.
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.
I have renamed the metric. However I think that keeping namespace separated is better, it that's fine for you.
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.
I preferred it named based on the package name.
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.
Should we change the package name?
Signed-off-by: Julien Pivotto <[email protected]>
Signed-off-by: Julien Pivotto <[email protected]>
4fc67c9
to
c652415
Compare
Would renaming the package to http help, like this? |
e633264
to
c652415
Compare
Signed-off-by: Julien Pivotto <[email protected]>
I did not like the "http" package rename as it is too close with go's net/http. I have reworked this to have a metric name: |
Signed-off-by: Julien Pivotto [email protected]