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.

Sign up for GitHub

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

Jump to bottom

datatrails: synchronize the range of the y axis for all breakdown panels #84781

Merged
merged 1 commit into from Mar 22, 2024
Merged

datatrails: synchronize the range of the y axis for all breakdown panels #84781

merged 1 commit into from Mar 22, 2024

Conversation

darrenjaneczek
Copy link
Contributor

What is this feature?

  • Ensures that all breakdown panels use the same y-axis range and markers so that they can be visually compared fairly
  • Remove the legend for panels with multiple series, so that the legend doesn't bump up the axis
  • Altered the tooltip to include legend content so the user can identify the series on hover
  • When the data changes, by either selecting a new time range, or selecting a different breakdown, the axis will adjust based on the visible data

image

image

image

Special notes for your reviewer:

  • Decide if we want to include a toggle to turn this behavior on and off.

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@darrenjaneczek darrenjaneczek added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Mar 19, 2024
@darrenjaneczek darrenjaneczek added this to the 11.0.x milestone Mar 19, 2024
@darrenjaneczek darrenjaneczek force-pushed the datatrails/breakdown-y-axis branch 3 times, most recently from c8b2775 to 39aa817 Compare March 20, 2024 02:42
darrenjaneczek
darrenjaneczek commented Mar 20, 2024
View reviewed changes
public/app/features/trails/ActionTabs/BreakdownScene.tsx Outdated
Comment on lines 263 to 288
const vizPanel = PanelBuilders.timeseries()
.setTitle(option.label!)
.setData(
new SceneQueryRunner({
maxDataPoints: 300,
datasource: trailDS,
queries: [
{
refId: 'A',
expr: expr,
legendFormat: `{{${option.label}}}`,
},
],
})
)
.setHeaderActions(new SelectLabelAction({ labelName: String(option.value) }))
.setUnit(unit)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is extrapolated out into a vizPanel reference so we can change the options.
Due to the ambiguous type, we must us onOptionsChange in an activation handler.

darrenjaneczek
darrenjaneczek commented Mar 20, 2024
View reviewed changes
public/app/features/trails/AutomaticMetricQueries/query-generators/histogram.ts Outdated
Comment on lines 72 to 73
// By including the specific refIds, the BreakdownScene can
// compare refIds to find a match
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 suppose this isn't necessary, now that I've removed the code that determines min and max from the main vizPanel queries.

@darrenjaneczek darrenjaneczek self-assigned this Mar 20, 2024
leeoniya
leeoniya reviewed Mar 20, 2024
View reviewed changes
public/app/features/trails/ActionTabs/BreakdownScene.tsx
Comment on lines +105 to +112
if (breakdownData.type !== FieldType.number) {
return;
}
Copy link
Contributor

@leeoniya leeoniya Mar 20, 2024 •
edited

Choose a reason for hiding this comment

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

maybe this should also ensure units match? or is this guaranteed?

prolly dont want latency on Y min/maxed with bytes on Y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Units are guaranteed to match. Each panel gets the same query with a slightly different label filter.

darrenjaneczek
darrenjaneczek commented Mar 20, 2024
View reviewed changes
public/app/features/trails/AutomaticMetricQueries/query-generators/histogram.ts
@@ -78,7 +69,7 @@ function percentileQuery(percentile: number, groupings: string[] = []) {
const percent = percentile / 100;

return {
refId: 'A',
refId: `Percentile${percentile}`,
expr: `histogram_quantile(${percent}, ${baseQuery(groupings)})`,
legendFormat: `${percentile}th Percentile`,
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 really hope this function is never used with a number that ends in {1, 2, 3}

@darrenjaneczek
Copy link
Contributor Author

@leeoniya , do you think in the future it might be possible for the underlying panel:

  • to optionally remove non-valued entries from the legend on tooltip?
  • What about setting a limit of values shown when sorting?

@leeoniya
Copy link
Contributor

leeoniya commented Mar 20, 2024 •
edited

to optionally remove non-valued entries from the legend on tooltip?

like all-zeros or all-nulls?

we added #64477 a while back for this purpose.

What about setting a limit of values shown when sorting?

there are ongoing discussions about how to handle too-long tooltips. we have an internal tooltip.maxHeight and tooltip.maxWidth panel options that we don't expose in panel edit yet that you can probably use to at least contain the damage. no concrete plans for something like maxItems tho, but feel free to open a feature request.

@darrenjaneczek
Copy link
Contributor Author

@leeoniya

like all-zeros or all-nulls?

we added #64477 a while back for this purpose.

All nulls.
Overrides! I'll try it.

@leeoniya
Copy link
Contributor

leeoniya commented Mar 20, 2024 •
edited

if you want to exclude from everywhere in the viz you might just be better off diy filtering these fields out before sending to the panel. you can still use this reducer matcher if you want to save 5 LoC 🤷

@darrenjaneczek
Copy link
Contributor Author

darrenjaneczek commented Mar 20, 2024 •
edited

@darrenjaneczek:

@leeoniya , do you think in the future it might be possible for the underlying panel:

  • to optionally remove non-valued entries from the legend on tooltip?
  • What about setting a limit of values shown when sorting?

It turns out that enabling the newVizTooltips feature flags, these requests are already possible in the present (but using maxHeight instead of max entries).

@grafanabot
Copy link
Contributor

❌ Failed to run Playwright plugin e2e tests.

Click here to browse the Playwright report and trace viewer.
For information on how to run Playwright tests locally, refer to the Developer guide.

@darrenjaneczek
Copy link
Contributor Author

This is how the tooltip legend looks with a height limit of 250 when the new tooltip viz.

image

@darrenjaneczek darrenjaneczek force-pushed the datatrails/breakdown-y-axis branch 3 times, most recently from 2b0f441 to 94b8cb4 Compare March 21, 2024 22:26
@darrenjaneczek darrenjaneczek merged commit 810c224 into main Mar 22, 2024
14 checks passed
@darrenjaneczek darrenjaneczek deleted the datatrails/breakdown-y-axis branch March 22, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leeoniya leeoniya leeoniya left review comments

@catherineymgui catherineymgui catherineymgui approved these changes

@adrapereira adrapereira Awaiting requested review from adrapereira

@grafanabot grafanabot Awaiting requested review from grafanabot grafanabot is a code owner

@torkelo torkelo Awaiting requested review from torkelo torkelo is a code owner

Assignees

@darrenjaneczek darrenjaneczek

Labels
area/frontend no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Milestone
11.0.0-preview
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@darrenjaneczek @leeoniya @grafanabot @catherineymgui @fabrizio-grafana