feat: Add internal telemetry prometheus exporter #1691
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1691 +/- ##
==========================================
- Coverage 84.08% 84.06% -0.02%
==========================================
Files 467 469 +2
Lines 137359 137651 +292
==========================================
+ Hits 115500 115721 +221
- Misses 21325 21396 +71
Partials 534 534
🚀 New features to boost your workflow:
|
rust/otap-dataflow/crates/config/src/pipeline/service/telemetry/metrics/readers/pull.rs
Show resolved
Hide resolved
...low/crates/telemetry/src/opentelemetry_client/meter_provider/prometheus_exporter_provider.rs
Outdated
Show resolved
Hide resolved
|
|
||
| let response = Response::builder() | ||
| .status(StatusCode::OK) | ||
| .header("Content-Type", "text/plain; version=0.0.4; charset=utf-8") |
There was a problem hiding this comment.
we may have to support openmetrics format in the future. Not something for this PR.
| let registry = Registry::new(); | ||
| registry | ||
| .register(Box::new( | ||
| prometheus::Counter::new("test_counter", "A test counter").unwrap(), |
There was a problem hiding this comment.
lets use OpenTelemetry counter to properly test the integration. Otherwise, we are just testing prometheus crate mostly here...
There was a problem hiding this comment.
This is testing the handler as a unit, that receives the registry as parameter. It is not an integration test.
drewrelmas
left a comment
drewrelmas
left a comment
There was a problem hiding this comment.
Only nit is keeping config/README.md up to date with support for prometheus
cijothomas
left a comment
cijothomas
left a comment
There was a problem hiding this comment.
LGTM. One thing to address soon is how this interacts with existing prometheus metrics endpoint (https://github.com/open-telemetry/otel-arrow/blob/main/rust/otap-dataflow/crates/admin/src/telemetry.rs#L209)
Need some discussions to get consensus on the correct path we want to pursue.
There was a problem hiding this comment.
Thanks for the work on this PR. I want to clarify that I'm approving it with reservations.
I am strongly in favor of using our pipeline engine to report our internal telemetry (more aligned with #1672 ). I see multiple advantages in this approach:
- it provides strong motivation to develop fundamental exporters, such as an OTAP-based Prometheus exporter,
- it enables native support for OTAP and, in the future, experimentation with protocol extensions to natively support multivariate metrics, which we already support internally,
- it makes it possible to leverage our future processing language, OPL, on our own internal telemetry.
Important: integrating Prometheus support via the SDK must not lead to the removal of the existing /metrics endpoint. F5 currently uses this endpoint (JSON and compact JSON formats) because of its optimized support for multivariate metrics. It is fine to remove the Prometheus format from this endpoint if desired.
Looking ahead, I think this type of development should be discussed upstream to avoid this kind of duplication. I'm fully aware that I've been absent from the last two to three SIG meetings, so I take my share of responsibility for that.
@jmacd @drewrelmas @albertlockett for visibility
4 participants
Add internal telemetry configurable prometheus exporter.