metrics-exporter-prometheus crashes if port 9000 blocked

[bryanlarsen]

If you have something already using port 9000, metrics-exporter-prometheus crashes at https://github.com/metrics-rs/metrics/blob/ca453d31009e1809161a0644943bbf5a8b3eb2e6/metrics-exporter-prometheus/src/exporter/builder.rs#L61, even if you never call install(), and even if you use with_http_listener to tell it to use a different port.

More info: https://github.com/Ptrskay3/axum-prometheus/issues/66

[tobz]

:wave:

I saw and responded on the linked issue, but just to dive in a little here:

If you have something already using port 9000, metrics-exporter-prometheus crashes..

This is expected since we can't bind to a port already in use.

, even if you never call install(), and even if you use with_http_listener to tell it to use a different port.

Now this part makes me pause. Do you have a reproduction for the "even if you tell it to use a different port" part? I'm not actually seeing how that would be possible.

[Ptrskay3]

I definitely can't reproduce that last part.

Changed the default http listener in axum-prometheus to:

 let (recorder, _) = PrometheusBuilder::new()
            .upkeep_timeout(Duration::from_secs(5))
            .with_http_listener(([0, 0, 0, 0], 9001))
            .unwrap()
            .build()
            .expect("Failed to build metrics recorder");

then ran netcat -l 9000, and my examples build and work just fine. Without this change they did panic because of the 9000 port already is use.

[bryanlarsen]

Ran into this again because I hadn't properly fixed it last time. I think I have it this time. But it does mean I cannot use a really nice axum_prometheus helper function.

Would it be possible to add a with_no_listener() builder function that axum_prometheus can use?

[tobz]

Without doing some sort of typestate builder to determine what gets returned/installed by the build/install methods... I don't having a configuration method like that could work.

It feels like what we could get away with is something like...

pub struct PrometheusBuilder {
    // Two builder methods that don't install anything:
    pub fn build(self) -> Result<(PrometheusRecorder, UpkeepFut, ExporterFut), BuildError>;
    pub fn build_recorder(self) -> Result<(PrometheusRecorder, UpkeepFut), BuildError>;

    // This method builds everything, installs the recorder, and spawns the upkeep future
    // and exporter future:
    pub fn install(self) -> Result<(), BuildError>;

    // This method builds everything, installs the recorder, and spawns the upkeep future
    // but returns a handle to the recorder for the caller to manually handle generating
    // the payloads:
    pub fn install_recorder(self) -> Result<PrometheusHandle, BuildError>;
}

I think install_recorder is what axum-prometheus would want, because it handles installing the recorder and spawning the upkeep task, but then returns the handle necessary for wiring up a custom scrape endpoint handler. For people looking to use Push Gateway support, axum-prometheus could conceivably drop down to the low-level build method and install/spawn as much or as little as they wanted to, deferring the rest to the user.

@Ptrskay3 How does that sound? I'm a little fuzzy these days on how axum-prometheus is integrating with metrics-exporter-prometheus, so my mental model might be off here.

[Ptrskay3]

That sounds reasonable to me. Currently we use the PrometheusBuilder::build method, but having the install_recorder API in your example seems even better.

The the push-gateway mode is not really thought out in axum-prometheus yet, so we just tell the user to drop down to the lower level APIs, and use the PrometheusBuilder themselves.