Add query planning CPU time stat #15318

weijiii · 2022-12-06T22:32:23Z

Description

This PR aims to close #2579

I think the thread in which planning begins does not need to be tracked when writing to QueryStateTimer#planningCpuTime. For DDL queries the whole state transitioning happens on the same thread. For normal queries beginPlanning and beginStarting are supposed to run on the same thread in a linear fashion, hence the first attempt to set planningCpuTime should yield correct result.

The following image shows the change on the UI. I suppose this should also be in QueryCompletedEvent so I will work on that and update this PR.

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

weijiii · 2022-12-07T02:48:35Z

Seems a system connector test is failing. Will fix.

weijiii · 2022-12-07T03:13:57Z

@sopel39 Can you confirm where we want to expose this stat? I think this stat is too detailed for runtime.query table. QueryCompletedEvent seems to be the right place. Thank you.

weijiii · 2022-12-07T05:50:03Z

Added item in trino-spi/pom.xml because number of parameters changed for QueryStatistics

weijiii · 2022-12-10T01:13:44Z

@sopel39 Hi, can you take a look? Thanks.

sopel39

lgtm % I think you should extract mysql event listener changes to separate (follow-up) PR

sopel39 · 2023-01-03T11:20:00Z

...sql-event-listener/src/main/java/io/trino/plugin/eventlistener/mysql/MysqlEventListener.java

@@ -131,6 +131,7 @@ public void queryCompleted(QueryCompletedEvent event)
                stats.getResourceWaitingTime().map(Duration::toMillis).orElse(0L),
                stats.getAnalysisTime().map(Duration::toMillis).orElse(0L),
                stats.getPlanningTime().map(Duration::toMillis).orElse(0L),
+                stats.getPlanningCpuTime().map(Duration::toMillis).orElse(0L),


Extract event listener changes to separate commit (maybe separate PR). I'm not sure if the changes here are not breaking changes for people who already setup mysql event listener.

@arhimondr could you take a look?

This change is braking. Though the MySQL listener is not designed to be used in production as of today and is built for testing purposes only. We don't have to maintain strong compatibility.

Thanks. In that case I thin MySQL listener should be out of scope of this PR

@sopel39 @arhimondr
Thanks for the review. I'd like to finish this PR. I will

Rebase w.r.t the latest commit

Extract the changes on trino-mysql-event-listener to a separate PR

arhimondr · 2023-01-03T16:22:59Z

...sql-event-listener/src/main/java/io/trino/plugin/eventlistener/mysql/MysqlEventListener.java

@@ -131,6 +131,7 @@ public void queryCompleted(QueryCompletedEvent event)
                stats.getResourceWaitingTime().map(Duration::toMillis).orElse(0L),
                stats.getAnalysisTime().map(Duration::toMillis).orElse(0L),
                stats.getPlanningTime().map(Duration::toMillis).orElse(0L),
+                stats.getPlanningCpuTime().map(Duration::toMillis).orElse(0L),


This change is braking. Though the MySQL listener is not designed to be used in production as of today and is built for testing purposes only. We don't have to maintain strong compatibility.

weijiii · 2023-04-01T17:28:59Z

I've rebased and extracted the changes on trino-mysql-event-listener to #16841
The failures are caused by the missing parts for trino-mysql-event-listener. Please take a look. Thanks!
cc: @sopel39 @arhimondr

sopel39 · 2023-04-03T08:03:31Z

The failures are caused by the missing parts for trino-mysql-event-listener

Could you make the build pass?

weijiii · 2023-04-03T20:03:57Z

The failures are caused by the missing parts for trino-mysql-event-listener

Could you make the build pass?

For all CI tasks to pass I will need to include the changes on mysql event listener. Having a separate commit for the mysql event listener part will also fail since the CI validates the build success at every commit. I've updated.

In that case I thin MySQL listener should be out of scope of this PR

Can I wait for all tests to pass then exclude the MYSQL listener part from this PR?

sopel39 · 2023-04-14T12:45:03Z

For all CI tasks to pass I will need to include the changes on mysql event listener.

I'm not sure why. You can have extra field in QueryStatistics, but not necessarily mysql changes as part of this PR. The UI change on it's own is very useful

weijiii · 2023-04-14T16:49:07Z

For all CI tasks to pass I will need to include the changes on mysql event listener.

I'm not sure why. You can have extra field in QueryStatistics, but not necessarily mysql changes as part of this PR. The UI change on it's own is very useful

Thanks let me try that.

weijiii · 2023-04-15T05:09:55Z

@sopel39 Rebased and excluded trino-mysql-event-listener DAO. Also updated #16841 based on this PR. Thanks.

sopel39

lgtm % comment

core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java

sopel39 · 2023-04-20T09:35:39Z

merged, thanks!

cla-bot bot added the cla-signed label Dec 6, 2022

weijiii marked this pull request as ready for review December 6, 2022 22:58

github-actions bot added the ui Web UI label Dec 6, 2022

weijiii requested a review from sopel39 December 7, 2022 00:23

weijiii force-pushed the expose_planning_cpu_stats branch from b321fe9 to 9797f96 Compare December 7, 2022 04:06

sopel39 approved these changes Jan 3, 2023

View reviewed changes

arhimondr approved these changes Jan 3, 2023

View reviewed changes

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

weijiii force-pushed the expose_planning_cpu_stats branch 3 times, most recently from 71efc3e to 9f7efe0 Compare April 1, 2023 09:09

weijiii mentioned this pull request Apr 1, 2023

Expose planning CPU time for MYSQL event listener #16841

Merged

weijiii force-pushed the expose_planning_cpu_stats branch from 9f7efe0 to c025060 Compare April 2, 2023 01:09

weijiii force-pushed the expose_planning_cpu_stats branch 3 times, most recently from 101e80e to 72cad60 Compare April 3, 2023 19:59

weijiii force-pushed the expose_planning_cpu_stats branch from 72cad60 to a8ae88b Compare April 15, 2023 05:08

weijiii force-pushed the expose_planning_cpu_stats branch from a8ae88b to 97fa11b Compare April 15, 2023 17:43

sopel39 approved these changes Apr 18, 2023

View reviewed changes

core/trino-main/src/main/java/io/trino/execution/QueryStateMachine.java Show resolved Hide resolved

Add planningCpuTime stats excluding mysql-event-listener DAO

8e07c78

weijiii force-pushed the expose_planning_cpu_stats branch from 97fa11b to 8e07c78 Compare April 18, 2023 20:11

sopel39 merged commit a524709 into trinodb:master Apr 20, 2023

sopel39 mentioned this pull request Apr 20, 2023

Release notes for 415 #17135

Closed

github-actions bot added this to the 415 milestone Apr 20, 2023

weijiii mentioned this pull request Apr 21, 2023

Move planning CPU time to resource utilization summary #17155

Merged

colebow mentioned this pull request Apr 26, 2023

Add Trino 415 release notes #17234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add query planning CPU time stat #15318

Add query planning CPU time stat #15318

weijiii commented Dec 6, 2022 •

edited

Loading

weijiii commented Dec 7, 2022

weijiii commented Dec 7, 2022 •

edited

Loading

weijiii commented Dec 7, 2022

weijiii commented Dec 10, 2022

sopel39 left a comment

sopel39 Jan 3, 2023

arhimondr Jan 3, 2023

sopel39 Jan 4, 2023

weijiii Apr 1, 2023

arhimondr Jan 3, 2023

weijiii commented Apr 1, 2023 •

edited

Loading

sopel39 commented Apr 3, 2023

weijiii commented Apr 3, 2023 •

edited

Loading

sopel39 commented Apr 14, 2023

weijiii commented Apr 14, 2023

weijiii commented Apr 15, 2023 •

edited

Loading

sopel39 left a comment

sopel39 commented Apr 20, 2023

Add query planning CPU time stat #15318

Add query planning CPU time stat #15318

Conversation

weijiii commented Dec 6, 2022 • edited Loading

Description

weijiii commented Dec 7, 2022

weijiii commented Dec 7, 2022 • edited Loading

weijiii commented Dec 7, 2022

weijiii commented Dec 10, 2022

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 Jan 3, 2023

Choose a reason for hiding this comment

arhimondr Jan 3, 2023

Choose a reason for hiding this comment

sopel39 Jan 4, 2023

Choose a reason for hiding this comment

weijiii Apr 1, 2023

Choose a reason for hiding this comment

arhimondr Jan 3, 2023

Choose a reason for hiding this comment

weijiii commented Apr 1, 2023 • edited Loading

sopel39 commented Apr 3, 2023

weijiii commented Apr 3, 2023 • edited Loading

sopel39 commented Apr 14, 2023

weijiii commented Apr 14, 2023

weijiii commented Apr 15, 2023 • edited Loading

sopel39 left a comment

Choose a reason for hiding this comment

sopel39 commented Apr 20, 2023

weijiii commented Dec 6, 2022 •

edited

Loading

weijiii commented Dec 7, 2022 •

edited

Loading

weijiii commented Apr 1, 2023 •

edited

Loading

weijiii commented Apr 3, 2023 •

edited

Loading

weijiii commented Apr 15, 2023 •

edited

Loading