-
Notifications
You must be signed in to change notification settings - Fork 26
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
Make the metrics registry configurable #134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nits
@@ -82,6 +82,7 @@ axum = { version = "0.6", features = ["tokio"] } | |||
criterion = "0.5" | |||
http = "0.2" | |||
opentelemetry_sdk = "0.19" | |||
prometheus-client = "0.21" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't optional
? you activate it with dep:prometheus-client
like 60 lines above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be in the dev dependencies because it's used in the examples in the docs, which fail to compile without that being there
CHANGELOG.md
Outdated
- `backends::prometheus_client::REGISTRY` was removed. The `Registry` can now be accessed as | ||
a property on the `AutometricsSettings` struct |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of view of the users, the registry is only accessible if they chose the prometheus-client
feature, I don't know if we should specify the feature in the CHANGELOG, or make the registry in AutometricsSettings::prometheus_registry
pub
instead of pub(crate)
, but it feels a little inconsistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a getter method on the settings for the prometheus::Registry
as well
Resolves #20