feat(prometheus-client-derive): initial implemenation #270
Conversation
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
ADD-SP
force-pushed
the
add_sp/feat-prometheus-client-derive
branch
from
74c6b17 to
a194c89
Compare
| @@ -24,6 +32,7 @@ parking_lot = "0.12" | |||
| prometheus-client-derive-encode = { version = "0.5.0", path = "derive-encode" } | |||
| prost = { version = "0.12.0", optional = true } | |||
| prost-types = { version = "0.12.0", optional = true } | |||
| prometheus-client-derive = { version = "0.24.0", path = "./prometheus-client-derive" } | |||
There was a problem hiding this comment.
Shall we gate this dependency using a feature flag?
If so, we should also gate the prometheus-client-derive-encode, which should be considered as a breaking change even if the feature flag is default on.
Since the latest release is 0.23.1, the current version is 0.24.0, we can do breaking changes, but this based on the policy of the project. So the project maintainer may able to make a better call.
Signed-off-by: ADD-SP <[email protected]>
| // unit should be lowercase | ||
| let unit = syn::LitStr::new( | ||
| unit.value().as_str().to_ascii_lowercase().as_str(), | ||
| unit.span(), | ||
| ); |
There was a problem hiding this comment.
There are two options:
- Silently covert it to lowercase .
- Emit compilation error.
Unit is a part of the metric name in Prometheus output, but Prometheus allow uppercase in name which is not the best practice.
What do you think?
| ); | ||
| attr.unit = Some(unit); | ||
| } else if meta.path.is_ident("rename") { | ||
| let rename = meta.value()?.parse::<syn::LitStr>()?; |
There was a problem hiding this comment.
Enforce "Names SHOULD be in snake_case".
From README.md
Shall we emit compilation error if the metric name is not snake_case?
| // public. | ||
| #[derive(Default)] | ||
| pub struct Attribute { | ||
| pub help: Option<syn::LitStr>, |
There was a problem hiding this comment.
Shall we make the help as a required field? Missing help message might make no sense.
Signed-off-by: ADD-SP <[email protected]>
Signed-off-by: ADD-SP <[email protected]>
|
Interesting idea, would you be interested in porting this PR to my openmetrics library? |
|
@mxinden Could you take a look? |
Summary
This PR implements features proposed by #140, supersedes #218 (which doesn't makes new progress in the past several months).
This PR adds and publishes (to
crates.io) a new crateprometheus-client-derive, and re-exports its macros in crateprometheus-client, so the user should useprometheus-clientinstead ofprometheus-client-derive. This re-export pattern is also used byserde.The new crate only has one macros
Registrantderive the trait that allows structs to be registered in a registry.Futures of
derive-encodeThis PR doesn't touch it, but I think we should merge it into
prometheus-client-deriveeventually as the first one's functionalities is a subset of the second one.Since this is a breaking change, this is not the call should be made by outside contributor, even if we can do breaking change when releasing
0.24.0based on the Rust semantic version.Issues
close #140, supersede #218