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

ui: improve timeperiod display #105157

Merged
merged 1 commit into from
Jun 21, 2023
Merged

Conversation

maryliag
Copy link
Contributor

Previously, the period being shown on SQL Activity page could be confusing, since we no longer refresh the page automatically. This could result in a scenario where a query is executed, the user click on Apply on the Search Criteria on X:05, but the page shows that the results goes up to X:59, but then if you executed new statements they won't show until Apply is clicked again, but because we still show the message that the results are up to X:59 this is misleading.
This commit updates the value being displayed to use the time the request was made, so we know the end window value, and even if the user changes page and go back, the value is still the X:05, making more obvious they need to click on Apply again if they want to see newer results.

Here an example of the previous behaviour on Transactions page and the new Behaviour on Statement page:
(to clarify, this PR make this update on Statement, Statement Details, Transaction and Transaction Details pages)
https://www.loom.com/share/ec19aa79a5144aea9e44bec59a5101a4

Epic: none
Release note: None

Previously, the period being shown on SQL Activity page
could be confusing, since we no longer refresh the page
automatically. This could result in a scenario where
a query is executed, the user click on Apply on the Search
Criteria on X:05, but the page shows that the results
goes up to X:59, but then if you executed new statements
they won't show until Apply is clicked again, but because
we still show the message that the results are up to X:59
this is misleading.
This commit updates the value being displayed to use the
time the request was made, so we know the end window value,
and even if the user changes page and go back, the value
is still the X:05, making more obvious they need to click
on Apply again if they want to see newer results.

Epic: none

Release note: None
@maryliag maryliag requested review from a team June 19, 2023 21:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag maryliag added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jun 19, 2023
Copy link

@dongniwang dongniwang left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes and tagging me!

I wonder when selecting "last hour", would user expect to see 1:23-2:23 or 2:00-2:23?

@maryliag
Copy link
Contributor Author

I wonder when selecting "last hour", would user expect to see 1:23-2:23 or 2:00-2:23?

it would be 1:00-2:23, because if you select past hour it would mean 1:23-2:23, but the 1:23 is aggregated on the 1:00, so we need to clarify that we show values starting on 1:00

Copy link
Contributor

@gtr gtr left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kevin-v-ngo)

@maryliag
Copy link
Contributor Author

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Jun 21, 2023

Build succeeded:

@craig craig bot merged commit 7cb2a89 into cockroachdb:master Jun 21, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jun 21, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from d74648f to blathers/backport-release-22.2-105157: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


error creating merge commit from d74648f to blathers/backport-release-23.1-105157: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@maryliag maryliag deleted the timescale-end branch June 21, 2023 21:20
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 11, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a new
parameter for requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 11, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a new
parameter for requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 11, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 12, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 12, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
craig bot pushed a commit that referenced this pull request Jul 13, 2023
106623: ui: fix timescale for rolling window r=maryliag a=maryliag

The change introduced on #105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.

Release note (bug fix): Statement Diagnostics not always 
showing is now fixed and they show for the correct time period selected.

106757: upgrademanager: skip TestMigrationFailure r=JeffSwenson a=JeffSwenson

Skip TestMigrationFailure until the source of flakes can be fixed.

Release note: None
Part of: #106648

Co-authored-by: maryliag <[email protected]>
Co-authored-by: Jeff <[email protected]>
maryliag added a commit to maryliag/cockroach that referenced this pull request Jul 13, 2023
The change introduced on cockroachdb#105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
maryliag added a commit that referenced this pull request Jul 24, 2023
The change introduced on #105157 had an undesired
change on the Metrics page. We want to show the end period
of the select, but when the fixed window end for that this
caused the Metrics page to stop loading new data.
We want the metrics page to keep updating when any of the
rolling windows is selected, and we also want to know
the time a time period was requested, so it can be used
to provide more information on SQL Activity pages.

This commit remove the setting of the fixedWindowEnd, since
that was not the best approach and instead creates a value
on local storage for the requestTime, that can be used to create
the label for the timescale, without interferring on
Metrics page (or any other that have an automatic update).

Epic: none

Release note (bug fix): Fix the Metrics page that was not
updating automatically on rolling window options.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants