Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configurable metrics #70

Merged
merged 10 commits into from
Dec 22, 2023
Merged

Configurable metrics #70

merged 10 commits into from
Dec 22, 2023

Conversation

potkae
Copy link
Contributor

@potkae potkae commented Mar 6, 2023

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
Copy link
Contributor Author

potkae commented Apr 17, 2023

@nlopes What do you think about this?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@vriesk
Copy link

vriesk commented Jul 6, 2023

@CommanderStorm friendly ping? :)

@CommanderStorm
Copy link

@CommanderStorm friendly ping? :)

Ping about what?

@vriesk
Copy link

vriesk commented Jul 6, 2023

@CommanderStorm friendly ping? :)

Ping about what?

@potkae's question in the comment above.

@CommanderStorm
Copy link

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
Copy link

vriesk commented Jul 6, 2023

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

@potkae
Copy link
Contributor Author

potkae commented Jul 7, 2023

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
Copy link
Owner

nlopes commented Jul 11, 2023

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.

src/lib.rs Outdated Show resolved Hide resolved
@nlopes
Copy link
Owner

nlopes commented Jul 21, 2023

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

@potkae
Copy link
Contributor Author

potkae commented Aug 9, 2023

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

@nlopes
Copy link
Owner

nlopes commented Sep 6, 2023

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

src/lib.rs Outdated Show resolved Hide resolved
@potkae
Copy link
Contributor Author

potkae commented Oct 9, 2023

@nlopes I'm not entirely sure how I would rebase this now with #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
Copy link
Contributor

mstyura commented Oct 10, 2023

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 potkae force-pushed the configurable-metrics branch 2 times, most recently from 71323e9 to 3bdd855 Compare October 26, 2023 14:58
@potkae
Copy link
Contributor Author

potkae commented Oct 26, 2023

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

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@potkae potkae requested a review from nlopes October 30, 2023 10:31
@vriesk
Copy link

vriesk commented Nov 24, 2023

@nlopes friendly ping :)

@potkae
Copy link
Contributor Author

potkae commented Nov 24, 2023

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

@potkae
Copy link
Contributor Author

potkae commented Dec 8, 2023

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

@nlopes nlopes merged commit 13fc0dd into nlopes:main Dec 22, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants