nlopes / actix-web-prom

Actix-web middleware to expose Prometheus metrics
MIT License
89 stars 56 forks source link

Configurable metrics #70

Closed potkae closed 8 months ago

potkae commented 1 year ago

This adds the possibility to rename metrics and the labels. The solution is extendable for other possible metric specific configuration as well as adding new metrics.

Example usage to rename a default metric

use actix_web_prom::{PrometheusMetricsBuilder, ActixMetricsConfiguration};

PrometheusMetricsBuilder::new("api")
    .endpoint("/metrics")
    .metrics_configuration(
        ActixMetricsConfiguration::default()
        .http_requests_duration_seconds_name("my_http_request_duration"),
    )
    .build()
    .unwrap();
potkae commented 1 year ago

@nlopes What do you think about this?

vriesk commented 1 year ago

@CommanderStorm friendly ping? :)

CommanderStorm commented 1 year ago

@CommanderStorm friendly ping? :)

Ping about what?

vriesk commented 1 year ago

@CommanderStorm friendly ping? :)

Ping about what?

@potkae's question in the comment above.

CommanderStorm commented 1 year ago

I am fine with the rationale. When I made that comment I was under the impression that adding labels is something uncommon => this is why I made the comments. Without the builder pattern the second change also makes less sense.

vriesk commented 1 year ago

Okay then - would it be possible to merge the PR now?

potkae commented 1 year ago

Okay then - would it be possible to merge the PR now?

Not sure if anyone except @nlopes has write access to merge? Would need his review.

nlopes commented 1 year ago

Apologies, been away from this for some time. I'm going to try my very best to deal with some of these PRs, including this one. Bear with me.

nlopes commented 1 year ago

If you don't mind rebasing and resolving the conflicts, I can then take a look. Thanks!

potkae commented 1 year ago

@nlopes Sorry for the delay, I was also away for a bit. It's now rebased & ready for review

nlopes commented 11 months ago

Thanks for doing that. Will try to review this week.

potkae commented 10 months ago

@nlopes I'm not entirely sure how I would rebase this now with https://github.com/nlopes/actix-web-prom/pull/74 merged. I think something like configure_labels function - only changing the labels for all default metrics - for ActixMetricsConfiguration struct could do the trick and enable both use cases, but that would be a breaking change compared to current main. Would you mind taking a look at it yourself? @mstyura would you have an opinion?

mstyura commented 10 months ago

As far as I see the changes I've introduced regarding http version label was not released yet, so it should not be a breaking change to modify implementation to configure http version in a way you are proposing in this PR.

potkae commented 10 months ago

@mstyura @nlopes This is now restructured on top of the latest main. Let me know what you think.

vriesk commented 9 months ago

@nlopes friendly ping :)

potkae commented 9 months ago

@nlopes I noticed some of your test runs were failing. cargo fmt linting fixes are now pushed.

potkae commented 8 months ago

@nlopes rebased this again, ready for review & merging.