Add support for the prometheus-client crate
#88
Conversation
|
For some reason, the tests are failing / flaky only when the |
|
I found out that #94 was the issue causing the tests to fail here. I opened a separate draft PR to work on fixing that issue, as it is separate from what's being added here. |
| @@ -186,9 +186,21 @@ pub use autometrics_macros::ResultLabels; | |||
| #[cfg(feature = "prometheus-exporter")] | |||
| pub use self::prometheus_exporter::*; | |||
|
|
|||
| /// Functionality specific to the libraries used to collect metrics | |||
| pub mod integrations { | |||
There was a problem hiding this comment.
Still not sure I like this name, but I can't think of anything better. metrics_libraries, producers, ...
There was a problem hiding this comment.
couldn't we just put the integrations as the top level name, getting rid of integrations? so the resolved path would end up being smth like autometrics::prometheus_client instead of autometrics::integrations::prometheus_client
There was a problem hiding this comment.
Hmm that would also be an option. The main downside with that is we may want some other submodules exported from the crate root that are different, and they could get a bit lost. For example #90. We'd have 4-5 modules that are all related to the metrics libraries and 1-2 that aren't.
|
We should also update the example for custom metrics to show how to do it with this crate. The only thing is that the way we are exporting the registry, you can't modify it to add more custom metrics to that same registry. That might not be a problem though, because this crate seems to discourage the use of global registries anyway. So maybe it's fine? |
| @@ -119,6 +122,10 @@ impl Objective { | |||
| } | |||
|
|
|||
| /// The percentage of requests that must meet the given criteria (success rate or latency). | |||
| #[cfg_attr( | |||
| feature = "prometheus-client", | |||
| derive(Clone, Debug, PartialEq, Eq, Hash) | |||
There was a problem hiding this comment.
can also be Copy, which would make it easier to use
There was a problem hiding this comment.
Hmm, true. Do you think we should derive all of those traits all of the time (not just when using prometheus-client)? I'm somewhat hesitant to do that proactively because it technically expands the surface area of the API and (maybe?) increases compile times. But maybe it's fine to just do it for this type of enum
There was a problem hiding this comment.
saving Copy makes it way nicer to use. just be sure that u dont plan to add more in the future which may not be Copy, as removing Copy is a breaking change and requires a major version bump (unlike adding it)
@mellowagain I think that's still undecided. My current thought on this PR is to try to get it in sooner, but fix some of those other things we've talked about in other PRs and wait until a bunch of them are done before publishing another version |
| @@ -186,9 +186,21 @@ pub use autometrics_macros::ResultLabels; | |||
| #[cfg(feature = "prometheus-exporter")] | |||
| pub use self::prometheus_exporter::*; | |||
|
|
|||
| /// Functionality specific to the libraries used to collect metrics | |||
| pub mod integrations { | |||
| @@ -119,6 +122,10 @@ impl Objective { | |||
| } | |||
|
|
|||
| /// The percentage of requests that must meet the given criteria (success rate or latency). | |||
| #[cfg_attr( | |||
| feature = "prometheus-client", | |||
| derive(Clone, Debug, PartialEq, Eq, Hash) | |||
There was a problem hiding this comment.
saving Copy makes it way nicer to use. just be sure that u dont plan to add more in the future which may not be Copy, as removing Copy is a breaking change and requires a major version bump (unlike adding it)
Resolves #25