Improve SQLMetric APIs, port existing metrics #908
Conversation
|
Here is an example of what is needed to use the new API in IOx: https://github.com/influxdata/influxdb_iox/pull/2385 |
There was a problem hiding this comment.
@andygrove @Dandandan @houqp I am sorry for the large PR -- though it is mostly doc comments / tests.
This PR incorporates the first round of feedback from @tustvold and @andygrove
on #901)
FYI @returnString (who wrote the initial metrics API)
| /// that had the same name and partition=`Some(..)` have been | ||
| /// aggregated together. The resulting `MetricsSet` has all | ||
| /// metrics with `Partition=None` | ||
| pub fn aggregate_by_partition(&self) -> Self { |
There was a problem hiding this comment.
Here is the code that can aggregate metrics by partition (and sum above is also useful)
|
Thanks @andygrove -- I plan to merge this in later today unless I anyone objects or would like more time to review. |
| @@ -2172,6 +2172,8 @@ async fn csv_explain_analyze() { | |||
| let formatted = arrow::util::pretty::pretty_format_batches(&actual).unwrap(); | |||
| let formatted = normalize_for_explain(&formatted); | |||
|
|
|||
| println!("ANALYZE EXPLAIN:\n{}", formatted); | |||
There was a problem hiding this comment.
Really it is more like a development tool. I will plan to remove it as a follow on PR.
|
For anyone else who is updating code that uses the existing |
|
Late to the party, this is really great work, thanks @alamb ! |
Which issue does this PR close?
Closes #679. See #901 for an earlier version with feedback from @tustvold and @andygrove
Rationale for this change
See the description on #679 (comment) for the full rationale, but the TLDR version is:
What changes are included in this PR?
SQLMetricAPI to be in its own module, have labels, know about partitions, and allow for real time inspectionSQLMetric-->Metricoutput_rows) rather than camel 🐫 case (outputRows) to conform to Rust expectations (see note from @andygrove on the reason the names were camelCase to begin with: Implement new metrics API / RFC #901 (comment))Are there any user-facing changes?
YES!
The
SQLMetric/MetricAPI is now totally different so any code that creates / usesSQLMetricswould have to be updated. The updates are fairly mechanical as you can see in this PR)Notes
In keeping with Rust's tradition of static typing, I also changed to using more strongly typed versions of the metric values to avoid mistakes such as adding a "time" to a counter value, as well as allowing other counter specific operations.
Also, as the metrics aren't specific to SQL (they apply to any ExecutionPlan, even if that plan was created via the DataFrame API or the LogicalPlanBuilder) I renamed
SQLMetric-->MetricNot included in this PR: