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

Use an interface instead of a concrete type #42

Merged
merged 2 commits into from
Mar 22, 2018

Conversation

pavelnikolov
Copy link
Contributor

I am using the following versions:

- package: github.com/prometheus/client_golang
  version: ~0.9.0-pre1
- package: github.com/uber/jaeger-client-go
  version: ^2.12.0
# only in my glide.lock file
- name: github.com/uber/jaeger-lib
  version: 4267858c0679cd4e47cefed8d7f70fd386cfb567

If we use a histogram and try to create a new tracer (using jaeger-client-go) with metrics, for example:

        var reg prometheus.Registerer
        // set the value of reg to some Prometheus registry here
	factory := jaegerprom.New(jaegerprom.WithRegisterer(reg))
	tracer, closer, err := config.New(appName, jaegercfg.Metrics(factory))

I get the following error:

vendor/github.com/uber/jaeger-lib/metrics/prometheus/factory.go:143:12: cannot use hv.WithLabelValues(f.tagsAsLabelValues(labelNames, tags)...) (type prometheus.Observer) as type prometheus.Histogram in field value:
	prometheus.Observer does not implement prometheus.Histogram (missing Collect method)

This PR fixes this issue, while all unit tests still pass. It just replaces a concrete type with a single-method interface.

Signed-off-by: Pavel Nikolov <pavel.nikolov@fairfaxmedia.com.au>
@pavelnikolov
Copy link
Contributor Author

pavelnikolov commented Mar 22, 2018

I noticed that you use version 0.8.0 of the prometheus client for Go... This is the reason why the build fails : (
There are two options to fix this:

  1. update the prometheus client version referenced by this project
  2. create a copy of the prometheus.Observer single-method interface

Please, advise.

@pavelnikolov
Copy link
Contributor Author

pavelnikolov commented Mar 22, 2018

I just read the comments on #33 it seems that this PR can solve this problem.
If you don't want to update the version of Prometheus, then I'd suggest creating an Observer interface with a single method Observe(v float64). Then use it instead of a histogram, for example:

type observer interface {
	Observe(v float64)
}

type timer struct {
	histogram observer
}

This would solve #33, without having to upgrade your dependencies to newer version of Prometheus. Once prometheus 0.9.0 is out you can delete this interface and replace it with prometheus.Observer. WDYT?

Signed-off-by: Pavel Nikolov <pavel.nikolov@fairfaxmedia.com.au>
@codecov
Copy link

codecov bot commented Mar 22, 2018

Codecov Report

Merging #42 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #42   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          25     25           
  Lines         710    710           
=====================================
  Hits          710    710
Impacted Files Coverage Δ
metrics/prometheus/factory.go 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bf560b...d55a606. Read the comment docs.

@pavelnikolov
Copy link
Contributor Author

/assign @yurishkuro

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice solution!

@yurishkuro yurishkuro merged commit c5ea3ab into jaegertracing:master Mar 22, 2018
@yurishkuro
Copy link
Member

Thanks, @pavelnikolov

@yurishkuro
Copy link
Member

@pavelnikolov a couple questions:

@pavelnikolov pavelnikolov deleted the use-interface branch March 22, 2018 21:48
@pavelnikolov
Copy link
Contributor Author

@yurishkuro

@yurishkuro
Copy link
Member

@pavelnikolov released as 1.5.0

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.

2 participants