Add metrics_rs integration for task metrics #100
Conversation
This commit moves the macros and traits used to implement the runtime metrics integration with metrics_rs to a separate module. These will be used in a later commit to implement the task metrics integration with metrics_rs. Works towards resolving tokio-rs#71
|
|
||
| macro_rules! metric_refs { | ||
| ( | ||
| [$struct_name:ident] [$($ignore:ident),* $(,)?] [$metrics_name:ty] [$emit_arg_type:ty] { |
There was a problem hiding this comment.
It's probably possible to derive the struct_name from the metrics_name by appending Ref, but that didn't seem too worth it. Let me know if you disagree.
There was a problem hiding this comment.
yea, better to pass the identifiers explicitly rather than constructing them
| #[cfg(tokio_unstable)] | ||
| impl MyMetricOp<&tokio::runtime::RuntimeMetrics> for (&metrics::Histogram, Vec<u64>) { | ||
| fn op(self, tokio: &tokio::runtime::RuntimeMetrics) { | ||
| for (i, bucket) in self.1.iter().enumerate() { | ||
| let range = tokio.poll_time_histogram_bucket_range(i); | ||
| if *bucket > 0 { | ||
| // emit using range.start to avoid very large numbers for open bucket | ||
| // FIXME: do we want to do something else here? | ||
| self.0 | ||
| .record_many(range.start.as_micros() as f64, *bucket as usize); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
To be honest, I was a little confused by this impl. It seems like it's specific to a single histogram metric in the runtime. Does this make it impossible to add any other histogram metrics in the future?
There was a problem hiding this comment.
If you will add more histograms you will need to give the Vec<u64> special names or something. There are enough ways of changing the macro to get this to work so I'm not that worried about it.
There was a problem hiding this comment.
Maybe we should rename the Histogram kind to a PollTimeHistogram kind for the meanwhile? (Not making this mandatory for approval)
There was a problem hiding this comment.
Good idea, I think that will help clarify. I've included this in my most recent commit.
This commit provides integration of task metrics and metrics.rs. Resolves tokio-rs#71
jkosh44
force-pushed
the
task-metrics-rs-integration
branch
from
cd1bdf5 to
65042bd
Compare
src/task/metrics_rs_integration.rs
Outdated
| /// then it is strongly recommended to use [`with_metrics_transformer`] to give each [`TaskMonitor`] | ||
| /// a unique metric name. |
There was a problem hiding this comment.
Maybe actually the metric name doesn't need to be unique as long as each TaskMonitor has some unique tags?
There was a problem hiding this comment.
with_metrics_transformer can add dimensions. Maybe this should be "a unique metric name or dimensions"?
There was a problem hiding this comment.
This definitely feels to me like a bad-ish API for the gauges, since if you have multiple TaskMonitors they will probably conflict in an annoying way.
There was a problem hiding this comment.
I've removed the default constructor in my most recent commit and make the metrics transformer mandatory.
|
Nice PR! I do think that API-wise, we don't want people to create task metrics without a transformer, since in that cause it is way too easy to end up with mixed gauges. Could you not add a default constructor for that? |
|
@arielb1 thanks so much for the review! I believe I've addressed all your comments in my most recent commit. Please let me know if I've missed something. |
src/task/metrics_rs_integration.rs
Outdated
| } | ||
| } | ||
|
|
||
| impl Default for TaskMetricsReporterBuilder { |
|
Could this functionality be published in a release? |
3 participants
This commit provides integration of task metrics and metrics.rs.
Resolves #71
I hope that no one was already working on this, I saw the issue was unassigned so I thought I'd give it a try.
I split the PR into two commits for easier reviewing. The first commit moves shared macros and traits to a separate module, but doesn't change any code (other than imports, exports, and visibility modifiers). The second commit actually adds the task metrics integration.
There is still some duplicate across the
XMetricsReporterBuilderandXMetricsReporterstructs. I played around with making a generic struct that can be used by both, but the structs themselves are fairly simple, so it didn't seem worth the added complexity. There is a decent amount of duplicated doc-comments across the structs though. Let me know if you have any thoughts.