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

Add prometheus bridge #4227

Merged
merged 7 commits into from
Sep 26, 2023
Merged

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Aug 21, 2023

Migrated from open-telemetry/opentelemetry-go#4351

This allows applications instrumented with the prometheus client library to use OpenTelemetry exporters (e.g. OTLP).

Limitations:

  • Summary metrics are dropped by the bridge. This could be fixed if we added the summary types to the Go SDK data model, or we could convert the summary to individual gauge and counter series.
  • Start times for histograms and counters are set to the process start time. This would only not be correct when using custom collectors, which is relatively uncommon. That will be fixed when the prometheus client allows recording start times for their instruments.
  • Exponential histograms are not yet supported.

Despite the limitations, I think this still has a lot of value to users.

@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #4227 (a48e595) into main (b39f96e) will increase coverage by 0.0%.
The diff coverage is 86.6%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4227    +/-   ##
======================================
  Coverage   82.1%   82.2%            
======================================
  Files        142     144     +2     
  Lines       9818   10005   +187     
======================================
+ Hits        8067    8229   +162     
- Misses      1617    1636    +19     
- Partials     134     140     +6     
Files Coverage Δ
bridges/prometheus/config.go 100.0% <100.0%> (ø)
bridges/prometheus/producer.go 85.3% <85.3%> (ø)

@dashpole
Copy link
Contributor Author

I've addressed open comments on the original PR.

@dashpole dashpole marked this pull request as ready for review August 21, 2023 20:59
@dashpole dashpole requested a review from a team August 21, 2023 20:59
@pirgeo
Copy link
Member

pirgeo commented Aug 22, 2023

Is there not a summary metric type in OTLP for exactly that reason? Or is it missing in the Go SDK data model?

@dashpole
Copy link
Contributor Author

It is missing in the Go SDK data model. Adding it to the data model would mean in-process exporters would have to handle summary-typed metrics. But maybe it is better to add it, and expect most exporters other than OTLP to drop it...

@pirgeo
Copy link
Member

pirgeo commented Aug 22, 2023

Got it!

Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

LGTM outside of some naming issue and module tracking in versions.yaml.

bridges/prometheus/go.mod Outdated Show resolved Hide resolved
bridges/prometheus/doc.go Outdated Show resolved Hide resolved
bridges/prometheus/go.mod Show resolved Hide resolved
bridges/prometheus/producer.go Outdated Show resolved Hide resolved
@dashpole
Copy link
Contributor Author

Rebased. I think this is good to go

Copy link
Member

@pellared pellared left a comment

Choose a reason for hiding this comment

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

I just left some comments for the documentation. Reference: https://github.com/open-telemetry/opentelemetry-go/blob/main/CONTRIBUTING.md#documentation

@dashpole let me know if you want me to merge this PR as it is.

Comment on lines +26 to +28
* Summary metrics are dropped by the bridge.
* Start times for histograms and counters are set to the process start time.
* It does not currently support exponential histograms.
Copy link
Member

Choose a reason for hiding this comment

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

Can we move it to doc.go?

Comment on lines +8 to +22
## Usage

```golang
// Make a Promethes bridge "Metric Producer" that adds metrics from the
// Prometheus DefaultGatherer. Add the WithGatherer(registry) option to add
// metrics from other registries.
bridge := prometheus.NewMetricProducer()
// Make a Periodic Reader to periodically gather metrics from the bridge, and
// push to an OpenTelemetry exporter.
reader := metric.NewPeriodicReader(otelExporter, metric.WithProducer(bridge))
// Create an OTel MeterProvider with our reader. Metrics from OpenTelemetry
// instruments are combined with metrics from Prometheus instruments in
// exported batches of metrics.
mp := metric.NewMeterProvider(metric.WithReader(reader))
```
Copy link
Member

Choose a reason for hiding this comment

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

We can make a "testable example" in example_test.go instead.

Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this file in favor of doc.go and example_test.go.

Comment on lines +5 to +6
The Prometheus Bridge allows using the Prometheus Golang client library
(github.com/prometheus/client_golang) with the OpenTelemetry SDK.
Copy link
Member

Choose a reason for hiding this comment

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

You can move it to doc.go.

@dashpole
Copy link
Contributor Author

I'd prefer to merge as-is. I'll open a follow-up with the new docs.

@pellared pellared merged commit 635c5bc into open-telemetry:main Sep 26, 2023
26 checks passed
@dashpole dashpole deleted the prometheus_bridge branch September 26, 2023 17:52
@MrAlias MrAlias added this to the v1.20.0/v0.45.0 milestone Sep 28, 2023
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.

5 participants