Open asmello opened 1 month ago
Apologies for the delayed response.
For the sake of avoiding misinformation percolating: the only reason it's currently possible to have multiple "installed" recorders is due to the nature of being pre-1.0, and how often (relatively speaking) we push minor versions for breaking changes. At 1.0, this footgun goes away completely.
As far as being able to get the contextual recorder: seems like it should be possible to do, following the footsteps of what tracing
does with Subscriber
and surfacing metrics::with_recorder
in the documentation (currently hidden).
I'd certainly be willing to review a PR adding something along those lines.
For the sake of avoiding misinformation percolating: the only reason it's currently possible to have multiple "installed" recorders is due to the nature of being pre-1.0, and how often (relatively speaking) we push minor versions for breaking changes. At 1.0, this footgun goes away completely.
I think I may have miscommunicated this part. I wasn't aware it's possible to have multiple "installed" recorders [1], and that's not really what I'm concerned about.
I was actually thinking more of a testing scenario again, where we want to isolate each test to use a separate Recorder instance. In this case you end up with multiple Recorders in the same binary (they'll all be local though).
Consider how this must work in practice:
fn main() {
let recorder = PrometheusBuilder::new().build_recorder();
let handle = recorder.handle();
metrics::with_local_recorder(&recorder, || {
let service = Service::builder().prometheus_handle(handle).build();
// ...
});
}
The "unsafe" part here is that the handle we pass down to use in the HTTP server may not match the local recorder that's in scope. Not a huge deal, but it's always nice to make invalid states non-representable.
As far as being able to get the contextual recorder: seems like it should be possible to do, following the footsteps of what tracing does with Subscriber and surfacing metrics::with_recorder in the documentation (currently hidden).
I think this would indeed solve the problem. Using this function, the previous example could become something like:
fn main() {
let recorder = PrometheusBuilder::new().build_recorder();
metrics::with_local_recorder(&recorder, || {
let service = Service::new();
// ...
});
}
// somewhere
impl Service {
fn setup_server() {
let routes = /* ... */;
metrics::with_recorder(|recorder| {
if let Some(pr) = recorder.downcast::<PrometheusRecorder>() {
let handle = pr.handle();
routes.register(metrics_endpoint(handle)); // or something similar
}
});
// ...
}
}
(Note that in this version the metrics endpoint is only enabled if the contextual recorder is a PrometheusRecorder
)
I'd certainly be willing to review a PR adding something along those lines.
Meaning just making metrics::with_recorder
public?
[1]: curious how that happens? Also I do think something like Layer
from tracing-subscriber
would be highly useful, but that's a separate discussion.
The "unsafe" part here is that the handle we pass down to use in the HTTP server may not match the local recorder that's in scope. Not a huge deal, but it's always nice to make invalid states non-representable.
Recorders are inherently specific to the thread they're used on when paired with metrics::with_local_recorder
. You can't pass an "incorrect" handle down unless you're creating the service on one thread (using with_local_recorder
, which sets recorder A) and then using that handle on another thread where recorder B is set, and expecting that recorder B is what the handle will access.
If you are running everything on a single thread, then everything should be fine... save for how difficult it is to get the handle where it needs to go, and thus the downcasting.
Meaning just making metrics::with_recorder public?
Yes.
Also I do think something like
Layer
fromtracing-subscriber
would be highly useful, but that's a separate discussion.
Already exists: https://docs.rs/metrics-util/latest/metrics_util/layers/index.html
Another one for ergonomics.
Scenario:
PrometheusHandle
.Currently, to make this work, we need to register the Prometheus Recorder and produce a handle to it in the very outer layers of the program (e.g. inside of
main()
) and then pass that all the way into the internal HTTP server state. Ideally we'd be able to get a handle internally by producing a reference to the contextual Recorder, downcasting it to the correct type and then callingPrometheusRecorder::handle
.This would actually be "safer", because if for some reason one creates multiple Recorders in the same program, there's a possibility of mismatch between the one in scope and the one being used to render the metrics. If we fetch the Recorder contextually that risk goes away.