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

Stop using removed prometheus.MustRegisterOrGet #3

Merged
merged 2 commits into from May 15, 2017

Conversation

fgrosse
Copy link
Contributor

@fgrosse fgrosse commented Mar 10, 2017

prometheus.MustRegisterOrGet is no longer available on the most recent master of https://github.com/prometheus/client_golang so the cmhttp package doesnt compile if that version is used (e.g. on a fresh go get github.com/classmarkets/cmhttp). This PR changes the code to use prometheus.MustRegister instead. Note that this breaks BC since it does no longer allow registering the same metric twice. We can probably find a way around this but I didnt want to invest to much time now since I do not use this decorator.

In hindsigt I think it was a mistake of adding the dependency to prometheus to this package. If you do not mind breaking BC then you could consider moving cmhttp.Instrumented and friends into a sub package.

@pschultz
Copy link
Member

pschultz commented Mar 10, 2017

How about promoting reqCnt, reqDur, reqSz and resSz to the package level, initializing them in the first call to InstrumentedWithOpts, and re-using them on subsequent calls? Then we maintain the current semantics.

package cmhttp

var metrics struct {
    reqCnt *prometheus.CounterVec
    reqDur prometheus.Summary
    // ...
}

@fgrosse
Copy link
Contributor Author

fgrosse commented May 11, 2017

Sorry for the long delay: I somehow forgot about this open MR 😓

@fgrosse fgrosse requested a review from pschultz May 11, 2017 08:38
@pschultz pschultz merged commit 238679f into classmarkets:master May 15, 2017
@pschultz
Copy link
Member

Thanks!

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.

None yet

2 participants