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: fix infinite re-render on the key visualizer page #106398

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

zachlite
Copy link
Contributor

@zachlite zachlite commented Jul 7, 2023

Since #101258, the TimeScaleDropdownWithSearchParams can cause infinite re-renders. The exact cause of the bug is not yet diagnosed, but it occurs on the key visualizer page, and seems to be related to the custom duration options. This is tracked in #106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams from the key visualizer, and replaces it with the vanilla TimeScaleDropdown. The custom duration options are still present.

Informs: #106395
Epic: none
Release note: None

Since cockroachdb#101258, the TimeScaleDropdownWithSearchParams
can cause infinite re-renders. The exact cause of the bug
is not yet diagnosed, but it occurs on the key visualizer page,
and seems to be related to the custom duration options. This
is tracked in cockroachdb#106395.

This commit removes the dependency on TimeScaleDropdownWithSearchParams
from the key visualizer, and replaces it with the vanilla TimeScaleDropdown.
The custom duration options are still present.

Informs: cockroachdb#106395
Epic: none
Release note: None
@zachlite zachlite added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Jul 7, 2023
@zachlite zachlite requested review from a team July 7, 2023 14:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)


pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 41 at r1 (raw file):

const IntervalSetting = "keyvisualizer.sample_interval";

const timeScaleOptions: TimeScaleOptions = {

can't you import this from cluster-ui since we have the options there already? Or is this means to be different (since I don't see the option for month and 2 months)?

Copy link
Contributor Author

@zachlite zachlite left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)


pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 41 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can't you import this from cluster-ui since we have the options there already? Or is this means to be different (since I don't see the option for month and 2 months)?

Since the key visualizer (currently) only persists samples for up to 1 week, it seemed strange to include duration options for anything longer.

In the other direction, the default smallest duration of 10 minutes felt too small since we'll only collect 2 samples by default in that time period. (1 sample every 5 minutes)

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @zachlite)


pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 41 at r1 (raw file):

Previously, zachlite wrote…

Since the key visualizer (currently) only persists samples for up to 1 week, it seemed strange to include duration options for anything longer.

In the other direction, the default smallest duration of 10 minutes felt too small since we'll only collect 2 samples by default in that time period. (1 sample every 5 minutes)

Got it! Thanks for clarifying!
Can you add a loom showing it working with those changes?

Copy link
Contributor Author

@zachlite zachlite left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)


pkg/ui/workspaces/db-console/src/views/keyVisualizer/index.tsx line 41 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

Got it! Thanks for clarifying!
Can you add a loom showing it working with those changes?

I increased the sample interval to 10 seconds just so it would be a little more interesting.
In the video, you can see how the samples displayed are a function of the time window selected.
At the time the video was taken, the key visualizer was running for a little more than 30 minutes, so when 1 hour is selected, you can see the total number of samples displayed grow slightly. (No behavior has changed)

https://www.loom.com/share/96f64efc518f4b69bae67b6a2940456f?sid=c4b86c8f-7250-4ade-964d-5090300ff1ee

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained

@zachlite
Copy link
Contributor Author

zachlite commented Jul 7, 2023

bors r+

@zachlite
Copy link
Contributor Author

zachlite commented Jul 7, 2023

TFTR!

@craig
Copy link
Contributor

craig bot commented Jul 7, 2023

Build succeeded:

@craig craig bot merged commit afe228e into cockroachdb:master Jul 7, 2023
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.

3 participants