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

Refactor Prometheus exporter #3239

Merged
merged 13 commits into from Oct 14, 2022

Conversation

MadVikingGod
Copy link
Contributor

@MadVikingGod MadVikingGod commented Sep 28, 2022

This change will automatically register the created exporter with prometheus, and provides a httpHandler for triggering collect.

This also adds the ability to choose which reader is used, defaulting to a Manual Reader.

Fix #3200, #3172

This change will automatically register the exporter with prometheus, and provides an httpHandler for triggering collect
@codecov
Copy link

codecov bot commented Sep 28, 2022

Codecov Report

Merging #3239 (297e130) into main (1e72af4) will increase coverage by 0.0%.
The diff coverage is 90.9%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #3239   +/-   ##
=====================================
  Coverage   77.7%   77.7%           
=====================================
  Files        163     164    +1     
  Lines      11238   11263   +25     
=====================================
+ Hits        8737    8762   +25     
  Misses      2303    2303           
  Partials     198     198           
Impacted Files Coverage Δ
exporters/prometheus/exporter.go 80.0% <82.3%> (+1.0%) ⬆️
exporters/prometheus/config.go 100.0% <100.0%> (ø)
sdk/trace/batch_span_processor.go 80.2% <0.0%> (-0.9%) ⬇️
exporters/jaeger/jaeger.go 91.1% <0.0%> (+0.8%) ⬆️

exporters/prometheus/config.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
Remote WithRegistry
Adds WithRegisterer and WithGatherer
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
exporters/prometheus/exporter.go Outdated Show resolved Hide resolved
@MadVikingGod
Copy link
Contributor Author

MadVikingGod commented Oct 7, 2022

When I remove both the WithGatherer and the ServceHTTP, we run into a problem that this is trying to solve. In the default path you must either use the client_go/prometheus to do create a registry, prometheus.NewRegistry(), or use the default gatherer, prometheus.DefaultGatherer, to be able to set up a promhttp server. Both of these have you import the client_go/prometheus which clashes with our prometheus.

I will go back to having a WithGatherer option and keeping the exporter as an HttpHandler.

@dashpole
Copy link
Contributor

dashpole commented Oct 7, 2022

Don't use prometheus.NewRegistry? The example should use promhttp.Handler(). No need to reference prometheus

@dashpole
Copy link
Contributor

dashpole commented Oct 7, 2022

(not promhttp.HandlerFor())

@MadVikingGod
Copy link
Contributor Author

I see. That does solve that problem.

Another unintended consequence of this change is by using the DefaultRegister we are now collecting the default prom metrics (go runtime stuff like go_gc_duration_seconds and go_goroutines etc. Do we want to include those?

@dashpole
Copy link
Contributor

dashpole commented Oct 7, 2022

Another unintended consequence of this change is by using the DefaultRegister we are now collecting the default prom metrics. Do we want to include those?

For a simple example it is probably OK, but it might cause confusion if someone used the otel runtime metrics at the same time. With our package being named prometheus, there isn't a way to both use a custom registry, and also use our exporter without import collisions. I think including the request handling prometheus metrics is a good thing.

An alternative would be to define our own NewRegistry() method which points at the upstream one so that users don't encounter import collisions when making their own registry. This is additive if we want to do it.

@MadVikingGod MadVikingGod linked an issue Oct 7, 2022 that may be closed by this pull request
CHANGELOG.md Outdated Show resolved Hide resolved
exporters/prometheus/confg_test.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Outdated Show resolved Hide resolved
exporters/prometheus/config.go Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants