-
Notifications
You must be signed in to change notification settings - Fork 90
feat: Add Summary metric type #254
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -6,6 +6,8 @@ pub mod family; | |||||||||||
pub mod gauge; | ||||||||||||
pub mod histogram; | ||||||||||||
pub mod info; | ||||||||||||
#[cfg(feature = "summary")] | ||||||||||||
pub mod summary; | ||||||||||||
Comment on lines
+9
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we add this line...
Suggested change
we'll also make sure that this shows up in the documentation generated on docs.rs, see: https://doc.rust-lang.org/rustdoc/unstable-features.html#doccfg-recording-what-platforms-or-features-are-required-for-code-to-be-present n.b. we'd need to also add this above in lib.rs, if it's not already present: #![cfg_attr(docsrs, feature(doc_cfg))] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I use this trick for my crates and I'm glad to include this. However, it seems a separated topic to this PR so I'd like to listen to maintainer's idea whether we should include it in this PR or add in a follow-up. Both works for me but different maintainers would have different preference. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fine either way. Preference for a separate pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. Then let's leave this PR as is. |
||||||||||||
|
||||||||||||
/// A metric that is aware of its Open Metrics metric type. | ||||||||||||
pub trait TypedMetric { | ||||||||||||
|
@@ -21,12 +23,13 @@ pub enum MetricType { | |||||||||||
Gauge, | ||||||||||||
Histogram, | ||||||||||||
Info, | ||||||||||||
#[cfg(feature = "summary")] | ||||||||||||
Summary, | ||||||||||||
Unknown, | ||||||||||||
// Not (yet) supported metric types. | ||||||||||||
// | ||||||||||||
// GaugeHistogram, | ||||||||||||
// StateSet, | ||||||||||||
// Summary | ||||||||||||
} | ||||||||||||
|
||||||||||||
impl MetricType { | ||||||||||||
|
@@ -37,6 +40,8 @@ impl MetricType { | |||||||||||
MetricType::Gauge => "gauge", | ||||||||||||
MetricType::Histogram => "histogram", | ||||||||||||
MetricType::Info => "info", | ||||||||||||
#[cfg(feature = "summary")] | ||||||||||||
MetricType::Summary => "summary", | ||||||||||||
MetricType::Unknown => "unknown", | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
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 am not familiar with
fastant
. Can you give a short summary why you chose it? I understand thatinstant
is deprecated.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.
We (the fastlabs members) maintain it and use it in large scale production env via fastrace. So far, so good.
It provides a monotonically non-decreasing clock, which is what is required here.