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

Query execution time #38999

Merged
merged 10 commits into from
Feb 23, 2024
Merged

Query execution time #38999

merged 10 commits into from
Feb 23, 2024

Conversation

kamilmielnik
Copy link
Contributor

@kamilmielnik kamilmielnik commented Feb 21, 2024

Closes #9408

Design doc: https://www.notion.so/metabase/Show-query-response-time-in-native-queries-ca4c47e01349463198446589848c2714

Slack discussion: https://metaboat.slack.com/archives/C0645JP1W81/p1708516928531709

Description

Shows query execution time for native queries in the footer of the query builder.

  • If the time is less than 1 second, the result is formatted as milliseconds (e.g. 123 ms)
  • If the time is at least 1 second, the result is formatted as seconds with 1 decimal place (e.g. 3.9 s)

Demo

2024-02-21.18-48-16.mp4

@kamilmielnik kamilmielnik added the no-backport Do not backport this PR to any branch label Feb 21, 2024
@kamilmielnik kamilmielnik marked this pull request as ready for review February 21, 2024 12:02
Copy link

replay-io bot commented Feb 21, 2024

Status Complete ↗︎
Commit c30adaa
Results
⚠️ 5 Flaky
2317 Passed

@kamilmielnik kamilmielnik changed the title Query execution time [WIP] Query execution time Feb 21, 2024
@kamilmielnik kamilmielnik changed the title [WIP] Query execution time Query execution time Feb 23, 2024
@kamilmielnik kamilmielnik requested a review from a team February 23, 2024 09:45
Copy link
Contributor

@uladzimirdev uladzimirdev left a comment

Choose a reason for hiding this comment

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

Minor comments

@@ -0,0 +1,9 @@
import { t } from "ttag";

export const formatDuration = (time: number): string => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we have other time utils in frontend/src/metabase/lib/formatting/time.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but none of them could be reused here and there already is a duration function - I didn't want to put another function like that in there, because it'd be hard to distinguish them.

So I decided it's best to keep it local to the component it's used in.

import { t } from "ttag";

import { Tooltip } from "metabase/ui";

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I believe there shouldn't be an empty line between those groups as both they both point out to the relative files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it starts with a dot it's relative, if it's absolute it's internal, so all is good here

Copy link
Contributor

Choose a reason for hiding this comment

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

that's not true, you know, it's a relative path with webpack paths mapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure about terminology here but this is what import/order enforces with the current config

expect(formatDuration(1425)).toBe("1.4 s");
expect(formatDuration(1475)).toBe("1.5 s");
expect(formatDuration(1500)).toBe("1.5 s");
expect(formatDuration(120000)).toBe("120.0 s");
Copy link
Contributor

Choose a reason for hiding this comment

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

.0 looks weird

Copy link
Contributor Author

@kamilmielnik kamilmielnik Feb 23, 2024

Choose a reason for hiding this comment

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

On the other hand 2.1 s and 2 s look inconsistent and would cause small layout shift when re-running the query

@kamilmielnik kamilmielnik merged commit 5776cf3 into master Feb 23, 2024
111 checks passed
@kamilmielnik kamilmielnik deleted the 9408-query-execution-time branch February 23, 2024 10:48
Copy link
Contributor

@kamilmielnik Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-backport Do not backport this PR to any branch .Team/Querying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Query response time
2 participants