Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

compare-perf: Add support for selecting a single run as input #3821

Merged

Conversation

tausbn
Copy link

@tausbn tausbn commented Nov 14, 2024

A very hacky implementation that simply instantiates an empty PerformanceOverviewScanner as the "from" column (i.e. with all values empty). To see it in action, select a single query run in the query history and pick "Compare Performance" from the context menu. Then select the "Single run" option when prompted.

Second commit takes care of simplifying the view when not in comparison mode.

@tausbn tausbn force-pushed the tausbn/compare-perf-add-single-run-view-option branch from 8b2f378 to 98910a4 Compare November 15, 2024 10:33
A very hacky implementation that simply instantiates an empty
`PerformanceOverviewScanner` as the "from" column (i.e. with all values
empty). To see it in action, select a single query run in the query
history and pick "Compare Performance" from the context menu. Then
select the "Single run" option when prompted.
@tausbn tausbn force-pushed the tausbn/compare-perf-add-single-run-view-option branch from 98910a4 to 73a865d Compare November 15, 2024 10:44
@tausbn tausbn requested review from asgerf and esbena November 15, 2024 10:46
@tausbn tausbn marked this pull request as ready for review November 15, 2024 10:46
@tausbn tausbn requested a review from a team as a code owner November 15, 2024 10:46
Comment on lines 540 to 547
<NumberHeader>{second != null && "After"}</NumberHeader>
<NumberHeader>
{first != null && second != null && "Delta"}
</NumberHeader>
{comparison && (
<NumberHeader>
{first != null &&
second != null &&
(comparison ? "Delta" : "Value")}
</NumberHeader>
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we hide Delta and do comparison ? "After" : "Value" instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it amounts to the same if it's only done in the header, it just looks weird 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually the check on comparison is dead as it's inside another check for comparison

Copy link
Author

Choose a reason for hiding this comment

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

Whoops! You're completely right. Brain was on autopilot.

@asgerf
Copy link
Contributor

asgerf commented Nov 15, 2024

Looks great! 🚢

Copy link
Contributor

@asgerf asgerf left a comment

Choose a reason for hiding this comment

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

The comment would be nice to fix but can be merged as is

@esbena
Copy link

esbena commented Nov 15, 2024

works for me locally with hack to support external logs as well, that should be trial-by-fire enough

@esbena
Copy link

esbena commented Nov 15, 2024

Let's merge this before #3819 - I know the semantic conflicts that have to be addressed.

@esbena
Copy link

esbena commented Nov 15, 2024

I'll immediately do some cleanups to this once it is merged. I have the commits ready locally..

@esbena esbena merged commit 5464a46 into hackathon/compare-perf Nov 15, 2024
13 checks passed
@esbena esbena deleted the tausbn/compare-perf-add-single-run-view-option branch November 15, 2024 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants