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: change start and end values on stmt details charts #118680

Merged
merged 1 commit into from Feb 6, 2024

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Feb 2, 2024

Previously, we were using the data values to define the start and end date of charts, which could cause confusion, since users might think that is a bug of now showing data, when instead there is no data to show.
This commit forces the start and end of the chart to be the selected period, this way we always show the correct period even if we don't have data to show.

Fixes #102214

https://www.loom.com/share/3f463d82407b4ab186a5b0c1e1ab2369

Release note (ui change): On Statement Details page always show the entire selected period, instead of just the period that had data.

@maryliag maryliag requested review from a team and xinhaoz and removed request for a team February 2, 2024 21:13
@maryliag maryliag requested a review from a team as a code owner February 2, 2024 21:13
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag
Copy link
Contributor Author

maryliag commented Feb 2, 2024

fyi @dt who created the issue (in case you're interested)

@maryliag maryliag added backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Feb 2, 2024
@maryliag maryliag requested a review from koorosh February 2, 2024 23:47
Copy link
Collaborator

@koorosh koorosh 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 awesome demo of the problem and how it works now!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/graphs/bargraph/index.tsx line 35 at r1 (raw file):

  uPlotOptions: Partial<Options>;
  yAxisUnits: AxisUnits;
  graphTsStartMillis?: number;

nit. I would suggest to define single prop that ensures that either both values or nothing is provided. ie:

xScale?: {
  startMillis: number,
  endMillis: number,
}

pkg/ui/workspaces/cluster-ui/src/graphs/bargraph/index.tsx line 59 at r1 (raw file):

    if (!alignedData) return;

    const start = graphTsStartMillis ? graphTsStartMillis : alignedData[0][0];

I suggest that it's necessary to validate that graphTsStartMillis is less than alignedData[0][0] to make sure it doesn't trim provided datapoints.
My concern that it might cause some misunderstanding why some data is missed on a chart.
Or at least we need explicitly articulate that this props take precedence over start and end timestamps in provided timeseries.

Copy link
Contributor Author

@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 @koorosh and @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/graphs/bargraph/index.tsx line 35 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

nit. I would suggest to define single prop that ensures that either both values or nothing is provided. ie:

xScale?: {
  startMillis: number,
  endMillis: number,
}

Done


pkg/ui/workspaces/cluster-ui/src/graphs/bargraph/index.tsx line 59 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

I suggest that it's necessary to validate that graphTsStartMillis is less than alignedData[0][0] to make sure it doesn't trim provided datapoints.
My concern that it might cause some misunderstanding why some data is missed on a chart.
Or at least we need explicitly articulate that this props take precedence over start and end timestamps in provided timeseries.

I want the xScale to take precedent, so I added the comment on the function

Copy link
Collaborator

@koorosh koorosh 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 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/ui/workspaces/cluster-ui/src/graphs/bargraph/index.tsx line 59 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I want the xScale to take precedent, so I added the comment on the function

Great thanks!

Previously, we were using the data values to define the start
and end date of charts, which could cause confusion, since
users might think that is a bug of now showing data, when
instead there is no data to show.
This commit forces the start and end of the chart to be the
selected period, this way we always show the correct period
even if we don't have data to show.

Fixes cockroachdb#102214

Release note (ui change): On Statement Details page always
show the entire selected period, instead of just the period
that had data.
@maryliag
Copy link
Contributor Author

maryliag commented Feb 5, 2024

TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Feb 5, 2024

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 6, 2024

Build succeeded:

@craig craig bot merged commit aee73ac into cockroachdb:master Feb 6, 2024
9 checks passed
Copy link

blathers-crl bot commented Feb 6, 2024

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 a251210 to blathers/backport-release-23.1-118680: 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 charts-period branch February 6, 2024 02:15
abarganier added a commit to abarganier/cockroach that referenced this pull request Mar 29, 2024
Fixes: cockroachdb#121362

cockroachdb#118680 introduced code to
align the timeseries charts on the statement details page to the time
range set by the time picker, instead of the time range that we had
available data for.

The code contained a bug where it would set the start & end of the
XScale to `Moment.prototype.valueOf` instead of the result of *calling*
`Moment.prototype.valueOf()`. This causes the start & end timestamps to
be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity
statement details page in DB Console will now render properly, after
fixing a bug related to setting the time range of the charts.
craig bot pushed a commit that referenced this pull request Mar 29, 2024
121366: pkg/ui: properly derive XScale in statement details page r=nkodali a=abarganier

Fixes: #121362

Epic: none

#118680 introduced code to align the timeseries charts on the statement details page to the time range set by the time picker, instead of the time range that we had available data for.

The code contained a bug where it would set the start & end of the XScale to `Moment.prototype.valueOf` instead of the result of *calling* `Moment.prototype.valueOf()`. This causes the start & end timestamps to be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity statement details page in DB Console will now render properly, after fixing a bug related to setting the time range of the charts.


121372: backupccl: plumb approximate file size in restore_span_covering r=dt a=msbutler

Epic: none

Release note: none

Co-authored-by: Alex Barganier <abarganier@cockroachlabs.com>
Co-authored-by: Michael Butler <butler@cockroachlabs.com>
blathers-crl bot pushed a commit that referenced this pull request Mar 29, 2024
Fixes: #121362

#118680 introduced code to
align the timeseries charts on the statement details page to the time
range set by the time picker, instead of the time range that we had
available data for.

The code contained a bug where it would set the start & end of the
XScale to `Moment.prototype.valueOf` instead of the result of *calling*
`Moment.prototype.valueOf()`. This causes the start & end timestamps to
be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity
statement details page in DB Console will now render properly, after
fixing a bug related to setting the time range of the charts.
blathers-crl bot pushed a commit that referenced this pull request Mar 29, 2024
Fixes: #121362

#118680 introduced code to
align the timeseries charts on the statement details page to the time
range set by the time picker, instead of the time range that we had
available data for.

The code contained a bug where it would set the start & end of the
XScale to `Moment.prototype.valueOf` instead of the result of *calling*
`Moment.prototype.valueOf()`. This causes the start & end timestamps to
be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity
statement details page in DB Console will now render properly, after
fixing a bug related to setting the time range of the charts.
abarganier added a commit to abarganier/cockroach that referenced this pull request Apr 1, 2024
Fixes: cockroachdb#121362

cockroachdb#118680 introduced code to
align the timeseries charts on the statement details page to the time
range set by the time picker, instead of the time range that we had
available data for.

The code contained a bug where it would set the start & end of the
XScale to `Moment.prototype.valueOf` instead of the result of *calling*
`Moment.prototype.valueOf()`. This causes the start & end timestamps to
be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity
statement details page in DB Console will now render properly, after
fixing a bug related to setting the time range of the charts.
abarganier added a commit to abarganier/cockroach that referenced this pull request Apr 4, 2024
Fixes: cockroachdb#121362

cockroachdb#118680 introduced code to
align the timeseries charts on the statement details page to the time
range set by the time picker, instead of the time range that we had
available data for.

The code contained a bug where it would set the start & end of the
XScale to `Moment.prototype.valueOf` instead of the result of *calling*
`Moment.prototype.valueOf()`. This causes the start & end timestamps to
be set to functions, instead of unix timestamps, which broke the charts.

This PR simply invokes the function.

Release note (bug fix): The timeseries graphs shown on the SQL Activity
statement details page in DB Console will now render properly, after
fixing a bug related to setting the time range of the charts.
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 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: SQL Activity stmt charts can be confusing for no-data periods
3 participants