-
Notifications
You must be signed in to change notification settings - Fork 1k
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 #4351
Add prometheus bridge #4351
Conversation
06bb868
to
de01c0f
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #4351 +/- ##
======================================
Coverage 83.4% 83.4%
======================================
Files 184 186 +2
Lines 14383 14569 +186
======================================
+ Hits 12002 12162 +160
- Misses 2153 2172 +19
- Partials 228 235 +7
|
c9dff79
to
ab50b0f
Compare
c528092
to
36b45cf
Compare
36b45cf
to
663ff6c
Compare
Epic work! We were actually discussing similar with Prometheus client maintainers. Not sure what form we all prefer and what's better for community, but perhaps it would make sense to either host or at least document such option on https://github.com/prometheus/client_golang ( As per created timestamp, we are working on it (feedback welcome): |
return otelCounter | ||
} | ||
|
||
func convertHistogram(metrics []*dto.Metric, now time.Time) metricdata.Histogram[float64] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not support Native Histograms (exponential ones): https://github.com/prometheus/client_model/blob/master/io/prometheus/client/metrics.proto#L82
That's fine, but let's provide clean error/warning and TODO (:
otelMetrics := make([]metricdata.Metrics, 0) | ||
for _, pm := range promMetrics { | ||
newMetric := metricdata.Metrics{ | ||
Name: pm.GetName(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this necessary to convert prometheus metric name to otlp metrics name?
For example convert http_request_duration_count
to http.request.duration.count
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it isn't feasible to do that, as otel can have underscores in names as well (e.g. my.very_special.metric
)
) | ||
|
||
// config contains options for the exporter. | ||
type config struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit: I think you mean "config contains options for the producer", not exporter
} | ||
|
||
// Option sets exporter option values. | ||
type Option interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here, exporter -> producer
@open-telemetry/proto-go-approvers I was thinking maybe this makes more sense in contrib? It isn't really a "core" part of what OTel requires from SDKs. |
I'm not opposed. What are your thoughts on hosting this in a personal/3rd party repository? |
I'm not a huge fan of personal repos, as it makes ownership changes harder on users. The other option would be for me to ask the prometheus maintainers to host this in a Prometheus-owned repository. When I last spoke with them, they were OK with that. If i'm initially maintaining the bridge, it is probably easiest for me to do so in an OTel repository, as i'm an approver. |
Yeah, contrib sounds good to me then if you plan to act as a maintainer for it 👍 |
Superseded by open-telemetry/opentelemetry-go-contrib#4227 |
This allows applications instrumented with the prometheus client library to use OpenTelemetry exporters (e.g. OTLP).
Limitations:
Despite the limitations, I think this still has a lot of value to users.