Skip to content

Add metrics for page filter and projection execution time#14135

Merged
raunaqmorarka merged 4 commits intotrinodb:masterfrom
raunaqmorarka:filter-time
Sep 24, 2022
Merged

Add metrics for page filter and projection execution time#14135
raunaqmorarka merged 4 commits intotrinodb:masterfrom
raunaqmorarka:filter-time

Conversation

@raunaqmorarka
Copy link
Copy Markdown
Member

@raunaqmorarka raunaqmorarka commented Sep 15, 2022

Description

Adds metrics for page filter and projection execution time to operator metrics which
are available in EXPLAIN ANALYZE VERBOSE.

Non-technical explanation

Add metrics for filter and projection execution time

Release notes

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

# General
* Add metrics for filter and projection execution time. ({issue}`14135`)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why double?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why are the two metrics being tracked differently? (recordXXX vs setXXX). That tells me there's something funny going on with the abstraction.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying re-use the recording of projection execution time by ExpressionProfiler here so that we don't need to call System.nanoTime twice for recording start and stop of projection.
I've tweaked the code to avoid using double and make the tracking more similar for both fields but it doesn't look exactly the same.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we not using the same mechanism for filters? Maybe we should do that first so that both use the same approach.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently PageProcessor does not perform profiling and tuning of batch sizes based on filter execution time, it does that only for projections. I'm not sure why that is the case or if adding if would be useful. If we need to add that, that seems more appropriate for a separate PR.

@raunaqmorarka raunaqmorarka force-pushed the filter-time branch 3 times, most recently from fa9496c to 698dce0 Compare September 20, 2022 03:41
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new shape is better, but I have a slight concern about the fact that both methods now look the same singnature-wise but behave very differently. I think this could be improved by renaming this method to recordFilterTimeSince to indicate that it's taking care of the "until" measurement.

Once we fix how we're measuring timing for filters (per my other comment), we can change it to look like the recordProjectionTime method below.

@raunaqmorarka raunaqmorarka merged commit f46fd9c into trinodb:master Sep 24, 2022
@raunaqmorarka raunaqmorarka deleted the filter-time branch September 24, 2022 09:53
@github-actions github-actions bot added this to the 398 milestone Sep 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants