Implement new metrics API / RFC #901

alamb · 2021-08-16T19:09:00Z

Which issue does this PR close?

Closes #679

Note: If people basically like this API I will go ahead and add unit tests for metrics.rs (e.g. for aggregate_by_partition)

Rationale for this change

See the description on #679 (comment) for the full rationale, but the TLDR version is:

Better align SQLMetric data model to ease integration in other metric systems (e.g. prometheus, influxdb, etc)
Ability to get per-partition metrics
Ability to get current metric values during execution

What changes are included in this PR?

Update the SQLMetric API to be in its own module, have labels, know about partitions, and allow for real time inspection
Update uses of SQLMetric in DataFusion and Ballista to the new API
Functionality to aggregate (sum) metrics via predicate and via partition

Are there any user-facing changes?

No

The SQLMetric API is basically now totally different so any code that creates / uses SQLMetrics would have to be updated.

Notes

In keeping with Rust's tradition of static typing, I also changed to using a more strongly typed version of these metrics to avoid mistakes such as adding a "time" to a counter value, as well as allowing other counter specific operations.

Open Questions:

The current SQL counters use "camel case" for the counter names (e.g. numRows) rather than the Rust standard "snake case" (e.g. num_rows). I kept the same naming convention in this PR, but I wonder if we want to make them more Rust standard snake case given we are messing with them all anyways.

Not included in this PR:

Ensure that all operators have reasonable metrics: Add "baseline" metrics to all built in operators #866
Support for a global "operator id" as described by @andygrove in Improved features and interoperability for SQLMetrics #679 (comment)

alamb · 2021-08-16T19:14:03Z

cc @tustvold would like your opinion on the suitability / consistency of the metrics.rs API in this PR to other metric APIs

alamb · 2021-08-16T19:37:04Z

datafusion/src/physical_plan/metrics.rs

+    /// Sums the values for metrics for which `f(metric)` returns
+    /// true, and returns the value. Returns None if no metrics match
+    /// the predicate.
+    pub fn sum<F>(&self, mut f: F) -> Option<usize>


Here are some of the aggregation primitives (sum and group by partition). I feel this API may grow as we understand the usecases more

tustvold

I've not looked in great detail at the instrumentation of the operators themselves, nor am I particularly familiar with what came before, but this makes a lot of sense to me. Very nice 👍

Edit: r.e. snake case vs camel case, FWIW most metrics systems I've interacted with don't support upper-case letters, nor hyphens, so snake case is pretty typical

datafusion/src/physical_plan/metrics.rs

tustvold · 2021-08-17T17:16:43Z

datafusion/src/physical_plan/metrics.rs

+    }
+
+    /// Add `n` to the metric's value
+    pub fn add(&self, n: usize) {


I like that metric recording doesn't involve any string manipulation 👍

ballista/rust/core/src/execution_plans/shuffle_writer.rs

datafusion/src/physical_plan/metrics.rs

tustvold · 2021-08-17T17:51:37Z

datafusion/src/physical_plan/metrics/wrappers.rs

+impl Time {
+    /// Create a new [`Time`] wrapper suitable for recording elapsed
+    /// times for operations.
+    pub fn new(inner: Arc<SQLMetric>) -> Self {


I wonder if these should be verifying the MetricKind of the SQLMetric? Or alternatively be private and have a member function on SQLMetric that does the verification

This is a good point. 🤔 though now I think about it the more Rust-y way of doing this would be to do it in the type system... so perhaps I will collapse MetricKind 🤔

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

alamb

THanks @tustvold -- I am going to implement the Cow approach and also try and consolidate SQLMetric and MetricKind to see how that looks

ballista/rust/core/src/execution_plans/shuffle_writer.rs

alamb · 2021-08-18T17:04:25Z

datafusion/src/physical_plan/metrics/wrappers.rs

+impl Time {
+    /// Create a new [`Time`] wrapper suitable for recording elapsed
+    /// times for operations.
+    pub fn new(inner: Arc<SQLMetric>) -> Self {


This is a good point. 🤔 though now I think about it the more Rust-y way of doing this would be to do it in the type system... so perhaps I will collapse MetricKind 🤔

alamb · 2021-08-18T19:06:57Z

@andygrove / @returnString as you implemented the SQLMetrics initially and have worked with them, do you have any context or opinion on the use of snake_case vs camelCase names for the metrics? For example numRows vs num_rows?

From this PR's original description:

The current SQL counters use "camel case" for the counter names (e.g. numRows) rather than the Rust standard "snake case" (e.g. num_rows). I kept the same naming convention in this PR, but I wonder if we want to make them more Rust standard snake case given we are messing with them all anyways.

@tustvold notes that

Edit: r.e. snake case vs camel case, FWIW most metrics systems I've interacted with don't support upper-case letters, nor hyphens, so snake case is pretty typical

Given that snake case is the standard in Rust as well, I would probably be inclined to update the metric names to use snake case as well

andygrove · 2021-08-18T19:11:06Z

Given that snake case is the standard in Rust as well, I would probably be inclined to update the metric names to use snake case as well

This makes sense. I didn't even think about the casing. I just spend too much time looking at Spark query plans so that influenced my initial work.

alamb · 2021-08-18T19:22:06Z

This makes sense. I didn't even think about the casing. I just spend too much time looking at Spark query plans so that influenced my initial work.

Thanks @andygrove -- I will then update this proposal to switch the names to snake_case then

alamb · 2021-08-18T19:22:51Z

I have some non trivial feedback to incorporate so marking this as a draft (and maybe I will open a new PR after updating the proposal)

andygrove

I don't have time to review in detail at the moment but I took a quick look through and LGTM

alamb · 2021-08-19T17:19:25Z

This PR has changed enough since initial feedback that I have opened a second one #908 with the updates. Closing this one.

alamb added the api change Changes the API exposed to users of the crate label Aug 16, 2021

github-actions bot added ballista labels Aug 16, 2021

alamb commented Aug 16, 2021

View reviewed changes

tustvold reviewed Aug 17, 2021

View reviewed changes

alamb and others added 7 commits August 18, 2021 12:47

Improve SQLMetric APIs, port existing metrics

11fe6b5

Update ballista for new interface

685a090

clippy

bafa042

Move Arc inside SharedMetricsSet

a49295a

Change MetricKind::Custom to use static str

abbd6cd

MetricKind implements Copy

098f07e

Apply suggestions from code review

0e1fce9

Co-authored-by: Raphael Taylor-Davies <1781103+tustvold@users.noreply.github.com>

alamb force-pushed the alamb/metrics branch from bec30d1 to 0e1fce9 Compare August 18, 2021 17:10

alamb commented Aug 18, 2021

View reviewed changes

alamb added 3 commits August 18, 2021 14:56

Use Cow<str> rather than Arc<str>

4cff244

clippy

337a6b0

Rename SharedMetricsSet to ExecutionPlanMetricsSet

03ba078

alamb marked this pull request as draft August 18, 2021 19:22

alamb added 6 commits August 18, 2021 15:45

Idomatic Rust: Use typed values rather than MetricKind

0d7bde5

checkpoint for compiling

cdf7a0d

Implement last bits

8b464f3

Break into modules

128c6c1

Rename SQLMetric -> Metric

b3b4aa0

fixup test

f0e4c62

use snake case

c07de69

andygrove reviewed Aug 19, 2021

View reviewed changes

alamb changed the title ~~Implement new metrics API~~ Implement new metrics API / RFC Aug 19, 2021

alamb mentioned this pull request Aug 19, 2021

Improve SQLMetric APIs, port existing metrics #908

Merged

alamb closed this Aug 19, 2021

alamb deleted the alamb/metrics branch August 8, 2023 20:12

H0TB0X420 pushed a commit to H0TB0X420/datafusion that referenced this pull request Oct 7, 2025

Update version and generate changelog (apache#901)

af2d665

Implement new metrics API / RFC #901

Implement new metrics API / RFC #901

Uh oh!

Conversation

alamb commented Aug 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Notes

Open Questions:

Not included in this PR:

Uh oh!

alamb commented Aug 16, 2021

Uh oh!

alamb Aug 16, 2021

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tustvold Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tustvold Aug 17, 2021

Choose a reason for hiding this comment

Uh oh!

alamb Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb Aug 18, 2021

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 18, 2021

Uh oh!

andygrove commented Aug 18, 2021

Uh oh!

alamb commented Aug 18, 2021

Uh oh!

alamb commented Aug 18, 2021

Uh oh!

andygrove left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Aug 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Aug 16, 2021 •

edited

Loading

tustvold left a comment •

edited

Loading