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

.*: update prometheus client, remove deprecated InstrumentHandler and InstrumentHandlerFunc methods #1314

Merged
merged 8 commits into from
Jul 11, 2019

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Jul 9, 2019

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

  • Updates prometheus/client_golang.
  • Removes usages of prometheus.InstrumentHandler and prometheus. InstrumentHandlerFunc
  • Introduces extprom/NewInstrumentedHandler and extprom/NewInstrumentedHandlerFunc.
  • Removes http_request_duration_microseconds (Summary).
  • Adds http_request_duration_seconds (Histogram).

New metrics:

# HELP http_request_duration_seconds Tracks the latencies for HTTP requests.
# TYPE http_request_duration_seconds histogram
# HELP http_request_size_bytes Tracks the size of HTTP requests.
# TYPE http_request_size_bytes summary
# HELP http_requests_total Tracks the number of HTTP requests.
# TYPE http_requests_total counter
# HELP http_response_size_bytes Tracks the size of HTTP responses.
# TYPE http_response_size_bytes summary

Old metrics:

# HELP http_request_duration_microseconds The HTTP request latencies in microseconds.
# TYPE http_request_duration_microseconds summary
# HELP http_request_size_bytes The HTTP request sizes in bytes.
# TYPE http_request_size_bytes summary
# HELP http_requests_total Total number of HTTP requests made.
# TYPE http_requests_total counter
# HELP http_response_size_bytes The HTTP response sizes in bytes.
# TYPE http_response_size_bytes summary

Verification

By using make test and manual testing against query API, it seems like previous behaviour kept intact.

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.

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?

Copy link
Contributor

@metalmatze metalmatze left a 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 🎉

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.

Some suggestions but overall awesome. Let me know what are your thoughts (:

CHANGELOG.md Outdated Show resolved Hide resolved
pkg/extprom/http/instrument_server.go Outdated Show resolved Hide resolved
pkg/extprom/http/instrument_server.go Outdated Show resolved Hide resolved

// ServerInstrumentor holds necessary metrics to instrument an http.Server
// and provides necessary behaviors
type ServerInstrumentor interface {
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member Author

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.

Copy link
Member

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 { ...

@brancz
Copy link
Member

brancz commented Jul 10, 2019

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.

@bwplotka bwplotka merged commit 34872e4 into thanos-io:master Jul 11, 2019
@bwplotka
Copy link
Member

Thanks! LGTM

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.

Move away from deprecated InstrumentHandler{,Func} and update to the newest version of client_golang
5 participants