-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
.*: update prometheus client, remove deprecated InstrumentHandler and InstrumentHandlerFunc methods #1314
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.
Awesome, thanks for this! 👍
Looking good from the first glance, but will review closer soon.
Can we add CHANGELOG for metric changes we are making here?
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.
Very nice job @kakkoyun 🎉
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.
Some suggestions but overall awesome. Let me know what are your thoughts (:
|
||
// ServerInstrumentor holds necessary metrics to instrument an http.Server | ||
// and provides necessary behaviors | ||
type ServerInstrumentor interface { |
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.
Also not sure of Instrumentor
name.. Isn't Middleware
bit better and clearer? Not a blocker, might be by personal preference, up to 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.
Maybe I am wrong but I think that if we would name these middlewares then there might be more confusion in the future when (if) we will switch to using Cortex's middlewares for caching.
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 wasn't also sure about Instrumentor
name, it's not even an actual word. I'm ok to use Middleware
.
@GiedriusS Since it will be under a particular namespace with package structure, maybe it won't create any confusion. What do you think?
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.
@bwplotka How about InstrumentationMiddleware
? It'll go like extpromhttp.NewInstrumentationMiddleware
, NewInstrumentationMiddleware().NewHandler()
etc.
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.
Maybe I am wrong but I think that if we would name these middlewares then there might be more confusion in the future when (if) we will switch to using Cortex's middlewares for caching.
Well that will be exactly the same middleware isn't it?
... handler http.Handler) http.HandlerFunc { ...
InstrumentationMiddleware sounds good to me. As the package and context are revolving around http, I think it's pretty clear, and doesn't cause confusion. |
Thanks! LGTM |
Updates
prometheus/client_golang
. Remove usages of deprecated methods by introducing wrapper methods which mimics behaviour the same behaviour using new APIs.Fixes #1304
Changes
prometheus/client_golang
.prometheus.InstrumentHandler
andprometheus. InstrumentHandlerFunc
extprom/NewInstrumentedHandler
andextprom/NewInstrumentedHandlerFunc
.http_request_duration_microseconds
(Summary).http_request_duration_seconds
(Histogram).New metrics:
Old metrics:
Verification
By using
make test
and manual testing against query API, it seems like previous behaviour kept intact.