Metrics exporter to Prometheus with OTLP #1438
Conversation
ipa-metrics/Cargo.toml
Outdated
| @@ -13,6 +13,11 @@ partitions = [] | |||
| crossbeam-channel = "0.5" | |||
| # This crate uses raw entry API that is unstable in stdlib | |||
| hashbrown = "0.15" | |||
| # Open telemetry crates: opentelemetry-prometheus crate implementation is based on Opentelemetry API and SDK 0.23. (TBC) | |||
| opentelemetry = "0.24" | |||
There was a problem hiding this comment.
this is great, but can you make a separate crate for it? ipa-metrics is intended to contain only core functionality and it is easier to maintain in the long term if it does not have extra dependencies
ipa-metrics/src/exporter.rs
Outdated
| use crate::{counter, install_new_thread, producer::Producer, MetricChannelType}; | ||
| struct MeteredScope<'scope, 'env: 'scope>(&'scope Scope<'scope, 'env>, Producer); | ||
|
|
||
| impl<'scope, 'env: 'scope> MeteredScope<'scope, 'env> { |
There was a problem hiding this comment.
you don't need this complexity, each thread has access to its own metric store throuh CurrentThreadMetricContext
| // Err(err) => Err(Error::application(StatusCode::INTERNAL_SERVER_ERROR, err)), | ||
| // } | ||
|
|
||
| // TODO: Remove this dummy metrics and get metrics for scraper from ipa-metrics::PrometheusMetricsExporter (see ipa-metrics/exporter.rs) |
There was a problem hiding this comment.
@akoshelev I'm still not clear how we can pull the metrics from the logging_handle. IpaHttpClient doesn't seem to have a reference to it?
There was a problem hiding this comment.
that's right, it is not referenced from anywhere. helper.rs does have logging_handle that has a reference to this controller. So we need to plumb it through to deliver Controller to the handlers
ipa-metrics/src/partitioned.rs
Outdated
| @@ -120,6 +121,10 @@ impl PartitionedStore { | |||
| self.get_mut(CurrentThreadContext::get()).counter(key) | |||
| } | |||
|
|
|||
| pub fn counters(&mut self) -> impl Iterator<Item = (&OwnedMetricName, CounterValue)> { | |||
There was a problem hiding this comment.
is &mut required here or &self can work too?
|
|
||
| /// Takes details from the HTTP request and creates a `[TransportCommand]::CreateQuery` that is sent | ||
| /// to the [`HttpTransport`]. | ||
| async fn handler(transport: Extension<MpcHttpTransport>) -> Result<Vec<u8>, Error> { |
There was a problem hiding this comment.
I'd encourage to write some tests here as it is often hard to figure out why HTTP server is not responding or responds with errors. There are some examples in this folder that exercise happy path and error path
| .merge(query::query_router(transport.clone())) | ||
| .merge(query::h2h_router(transport.clone())), | ||
| ) | ||
| .merge(query::metric_router(transport)) |
There was a problem hiding this comment.
Nit: Does this handler belong inside the query module? You can perhaps park it alongside echo
| let mut buff = Vec::new(); | ||
| store.export(&mut buff); | ||
|
|
||
| let result = String::from_utf8(buff).unwrap(); |
There was a problem hiding this comment.
It would be good to validate the results somehow. Also consider removing the println.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1438 +/- ##
==========================================
- Coverage 93.28% 93.05% -0.23%
==========================================
Files 239 239
Lines 43532 43652 +120
==========================================
+ Hits 40608 40621 +13
- Misses 2924 3031 +107 ☔ View full report in Codecov by Sentry. |
Added an exporter to Prometheus with opentelemetry