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

feat(DiffView): Add CTAs and comparison presets #231

Merged
merged 58 commits into from
Oct 30, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
58 commits
Select commit Hold shift + click to select a range
6fbc0b4
refactor(AppHeader): Split responsibilities
grafakus Oct 18, 2024
1725337
fix: Header now receives only the props it uses and not the whole model
grafakus Oct 18, 2024
ee7cd91
chore: Missing deconstruct
grafakus Oct 18, 2024
7ba7ad5
feat(AppHeader): Revamp
grafakus Oct 18, 2024
b6a5afb
feat(Page): Reduce top padding
grafakus Oct 18, 2024
5366824
feat: WiP
grafakus Oct 21, 2024
331b2f7
Merge branch 'main' into feat/revamp-app-header
grafakus Oct 21, 2024
2f895a3
Merge branch 'main' into feat/consistent-header
grafakus Oct 21, 2024
66b711a
feat: WiP
grafakus Oct 21, 2024
b3e3d02
fix: ...
grafakus Oct 21, 2024
4729ac0
fix: ...
grafakus Oct 21, 2024
2e98733
fix: ...
grafakus Oct 21, 2024
416cb53
test(E2E): Update screenshot
grafakus Oct 21, 2024
b58f288
fix(QueryBuilder): Fix width in diff view
grafakus Oct 21, 2024
df9b745
feat(DiffFlameGraph): Add presets
grafakus Oct 21, 2024
7cb4fe2
feat: Add save modal for saving user presets
grafakus Oct 21, 2024
0ba3815
feat: WiP
grafakus Oct 22, 2024
a2b4a9f
feat: Add crosshair cursor on timeseries
grafakus Oct 22, 2024
783aceb
Merge branch 'main' into feat/consistent-header
grafakus Oct 22, 2024
3c17ae6
Merge branch 'feat/consistent-header' into feat/diff-presets
grafakus Oct 22, 2024
5ea55f7
feat: Tweaks
grafakus Oct 22, 2024
1659c0f
test(EndToEnd): Update
grafakus Oct 22, 2024
1844ff0
Merge branch 'feat/consistent-header' into feat/diff-presets
grafakus Oct 22, 2024
74d8eba
feat: WiP
grafakus Oct 23, 2024
089e340
feat: Functional preset selection
grafakus Oct 23, 2024
eee3deb
chore: Modal copy
grafakus Oct 23, 2024
48a79df
feat: Add dummy custom preset
grafakus Oct 23, 2024
c4b338b
refactor(Header): CSS update
grafakus Oct 23, 2024
4057904
feat: Clear presets when time range changes
grafakus Oct 23, 2024
f092ab7
feat: CSS
grafakus Oct 23, 2024
39f6692
chore: CSS tweaks
grafakus Oct 23, 2024
f93e77b
fix: revert CSS tweak
grafakus Oct 23, 2024
6582417
feat: Tweak
grafakus Oct 23, 2024
a34c41a
feat: WiP CTAs
grafakus Oct 23, 2024
c529f0c
feat: Auto-select
grafakus Oct 23, 2024
ae0421a
chore: Add comments
grafakus Oct 23, 2024
845acca
chore(Variables): Cosmetics for error state
grafakus Oct 24, 2024
1dfe0c4
chore: Cosmetics
grafakus Oct 24, 2024
e872b51
Merge branch 'feat/consistent-header' into feat/diff-presets
grafakus Oct 24, 2024
a8f762e
fix(SceneComparePanel): Fix title update
grafakus Oct 24, 2024
91fd076
Merge branch 'main' into feat/diff-presets
grafakus Oct 25, 2024
3f7611e
chore: Remove console.log
grafakus Oct 25, 2024
447fd4a
fix(EndToEnd): Update screenshots++
grafakus Oct 25, 2024
44bbced
fix(EndToEnd): update screenshots
grafakus Oct 25, 2024
a7fe893
chore: Update presets label
grafakus Oct 25, 2024
347c3b2
feat: Add full range 1h preset
grafakus Oct 25, 2024
7a09cab
Merge branch 'main' into feat/diff-presets
grafakus Oct 25, 2024
8d3f4b2
fix: Import
grafakus Oct 25, 2024
cc29365
chore: Remove comment
grafakus Oct 28, 2024
7fcb139
chore(PyroscopeLogo): Better props
grafakus Oct 28, 2024
2ebf4ff
fix: Fix preset labels
grafakus Oct 28, 2024
e3c3434
feat: Reset preset on DS and/or service change
grafakus Oct 29, 2024
1bc8580
Merge branch 'main' into feat/diff-presets
grafakus Oct 30, 2024
b82c048
chore: Add meaningful comment
grafakus Oct 30, 2024
73b7432
fix: Track missing event + refactor
grafakus Oct 30, 2024
df4bb18
feat: Adjust presets
grafakus Oct 30, 2024
af51091
chore: Rename event
grafakus Oct 30, 2024
324a052
feat: Add whole range preset
grafakus Oct 30, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions e2e/tests/diff-flame-graph-view/diff-flame-graph.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,6 @@ test.describe('Diff flame graph view', () => {
await exploreProfilesPage.assertSelectedService('pyroscope');
await exploreProfilesPage.mouse.move(0, 0); // prevents the time picker tooltip to appear on the screenshot

const diffFlameGraphPanel = exploreProfilesPage.getByTestId('diff-flame-graph-panel');
await expect(diffFlameGraphPanel).toContainText(
'Select both the baseline and the comparison flame graph time ranges to view the diff flame graph'
);

await expect(exploreProfilesPage.getSceneBody()).toHaveScreenshot({
stylePath: './e2e/fixtures/css/hide-all-controls.css',
});
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { GrafanaTheme2 } from '@grafana/data';
import { Badge, Icon, useStyles2 } from '@grafana/ui';
import React from 'react';

const FEEDBACK_FORM_URL = 'https://grafana.qualtrics.com/jfe/form/SV_6Gav4IUU6jcYfd4';
export const FEEDBACK_FORM_URL = 'https://grafana.qualtrics.com/jfe/form/SV_6Gav4IUU6jcYfd4';

// borrowed from https://github.com/grafana/explore-logs/blob/main/src/Components/IndexScene/GiveFeedbackButton.tsx
export const GiveFeedbackButton = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,16 @@ import { ServiceNameVariable } from '../../domain/variables/ServiceNameVariable/
import { CompareTarget } from '../SceneExploreServiceLabels/components/SceneGroupByLabels/components/SceneLabelValuesGrid/domain/types';
import { SceneComparePanel } from './components/SceneComparePanel/SceneComparePanel';
import { SceneDiffFlameGraph } from './components/SceneDiffFlameGraph/SceneDiffFlameGraph';
import { ScenePresetsPicker } from './components/ScenePresetsPicker/ScenePresetsPicker';
import { syncYAxis } from './domain/behaviours/syncYAxis';
import { EventDiffAutoSelect } from './domain/events/EventDiffAutoSelect';
import { EventDiffChoosePreset } from './domain/events/EventDiffChoosePreset';

interface SceneExploreDiffFlameGraphState extends SceneObjectState {
baselinePanel: SceneComparePanel;
comparisonPanel: SceneComparePanel;
body: SceneDiffFlameGraph;
presetsPicker: ScenePresetsPicker;
}

export class SceneExploreDiffFlameGraph extends SceneObjectBase<SceneExploreDiffFlameGraphState> {
Expand All @@ -38,6 +42,7 @@ export class SceneExploreDiffFlameGraph extends SceneObjectBase<SceneExploreDiff
syncYAxis(),
],
body: new SceneDiffFlameGraph(),
presetsPicker: new ScenePresetsPicker(),
});

this.addActivationHandler(this.onActivate.bind(this));
Expand All @@ -54,18 +59,42 @@ export class SceneExploreDiffFlameGraph extends SceneObjectBase<SceneExploreDiff
profileMetricVariable.setState({ query: ProfileMetricVariable.QUERY_SERVICE_NAME_DEPENDENT });
profileMetricVariable.update(true);

this.subscribeToEvents();

return () => {
profileMetricVariable.setState({ query: ProfileMetricVariable.QUERY_DEFAULT });
profileMetricVariable.update(true);
};
}

subscribeToEvents() {
this._subs.add(
this.subscribeToEvent(EventDiffAutoSelect, (event) => {
this.autoSelectDiffRanges(event.payload.wholeRange);
})
);

this._subs.add(
this.subscribeToEvent(EventDiffChoosePreset, () => {
this.state.presetsPicker.openSelect();
})
);
}

autoSelectDiffRanges(selectWholeRange: boolean) {
const { baselinePanel, comparisonPanel } = this.state;

baselinePanel.autoSelectDiffRange(selectWholeRange);
comparisonPanel.autoSelectDiffRange(selectWholeRange);
}

// see SceneProfilesExplorer
getVariablesAndGridControls() {
return {
variables: [
sceneGraph.findByKeyAndType(this, 'serviceName', ServiceNameVariable),
sceneGraph.findByKeyAndType(this, 'profileMetricId', ProfileMetricVariable),
this.state.presetsPicker,
],
gridControls: [],
};
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
import { css } from '@emotion/css';
import { dateTimeFormat, FieldMatcherID, getValueFormat, GrafanaTheme2, systemDateFormats } from '@grafana/data';
import {
dateTime,
dateTimeFormat,
FieldMatcherID,
getValueFormat,
GrafanaTheme2,
systemDateFormats,
} from '@grafana/data';
import {
SceneComponentProps,
SceneDataTransformer,
Expand All @@ -17,7 +24,7 @@ import { getProfileMetric, ProfileMetricId } from '@shared/infrastructure/profil
import { omit } from 'lodash';
import React from 'react';

import { getDefaultTimeRange } from '../../../../domain/getDefaultTimeRange';
import { buildTimeRange } from '../../../../domain/buildTimeRange';
import { FiltersVariable } from '../../../../domain/variables/FiltersVariable/FiltersVariable';
import { getSceneVariableValue } from '../../../../helpers/getSceneVariableValue';
import { getSeriesStatsValue } from '../../../../infrastructure/helpers/getSeriesStatsValue';
Expand All @@ -26,6 +33,7 @@ import { PanelType } from '../../../SceneByVariableRepeaterGrid/components/Scene
import { addRefId, addStats } from '../../../SceneByVariableRepeaterGrid/infrastructure/data-transformations';
import { CompareTarget } from '../../../SceneExploreServiceLabels/components/SceneGroupByLabels/components/SceneLabelValuesGrid/domain/types';
import { SceneLabelValuesTimeseries } from '../../../SceneLabelValuesTimeseries';
import { Preset } from '../ScenePresetsPicker/ScenePresetsPicker';
import {
SceneTimeRangeWithAnnotations,
TimeRangeWithAnnotationsMode,
Expand Down Expand Up @@ -76,7 +84,7 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
filterKey,
title,
color,
$timeRange: new SceneTimeRange({ key: `${target}-panel-timerange`, ...getDefaultTimeRange() }),
$timeRange: new SceneTimeRange({ key: `${target}-panel-timerange`, ...buildTimeRange('now-1h', 'now') }),
timePicker: new SceneTimePicker({ isOnCanvas: true }),
refreshPicker: new SceneRefreshPicker({ isOnCanvas: true }),
timeseriesPanel: SceneComparePanel.buildTimeSeriesPanel({ target, filterKey, title, color }),
Expand Down Expand Up @@ -120,18 +128,18 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
const allValuesSum = getSeriesStatsValue(s, 'allValuesSum') || 0;
const formattedValue = getValueFormat(metricField.config.unit)(allValuesSum);
const total = `${formattedValue.text}${formattedValue.suffix}`;
const [diffFrom, diffTo, timeZone] = SceneComparePanel.getFlameGraphRange(timeseriesPanel);
const [diffFrom, diffTo, timeZone] = SceneComparePanel.getDiffRange(timeseriesPanel);

const displayName =
diffFrom && diffTo
? `${title} total = ${total} / Flame graph range = ${dateTimeFormat(diffFrom, {
? `Total = ${total} / Flame graph range = ${dateTimeFormat(diffFrom, {
format: systemDateFormats.fullDate,
timeZone,
})} → ${dateTimeFormat(diffTo, {
format: systemDateFormats.fullDate,
timeZone,
})}`
: `${title} total = ${total}`;
: `Total = ${total}`;

return {
matcher: { id: FieldMatcherID.byFrameRefID, options: s.refId },
Expand Down Expand Up @@ -163,7 +171,7 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
return timeseriesPanel;
}

static getFlameGraphRange(
static getDiffRange(
timeseriesPanel: SceneLabelValuesTimeseries
): [number | undefined, number | undefined, string | undefined] {
let diffFrom: number | undefined;
Expand All @@ -189,7 +197,7 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
}

subscribeToEvents() {
return this.subscribeToEvent(EventSwitchTimerangeSelectionMode, (event) => {
const switchSub = this.subscribeToEvent(EventSwitchTimerangeSelectionMode, (event) => {
// this triggers a timeseries request to the API
// TODO: caching?
(this.state.timeseriesPanel.state.body.state.$timeRange as SceneTimeRangeWithAnnotations).setState({
Expand All @@ -199,6 +207,19 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
: TimeRangeWithAnnotationsMode.DEFAULT,
});
});

const timeRangeSub = this.state.$timeRange.subscribeToState((newState, prevState) => {
if (newState.from !== prevState.from || newState.to !== prevState.to) {
this.updateTitle('');
}
});

return {
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (non-blocking): Automatically clean up subscriptions

I think in scenes when you use this._subs.add(...) it will automatically clean up all subscriptions when a scene is deactivated. Anything added with this.subscribeToEvent(...) should also be removed automatically:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for this! I always wanted to refactor it ; I'll do it in a future PR.

unsubscribe() {
timeRangeSub.unsubscribe();
switchSub.unsubscribe();
},
};
}

buildTimeseriesTitle() {
Expand All @@ -211,16 +232,76 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
return (this.state.timeseriesPanel.state.body.state.$timeRange as SceneTimeRangeWithAnnotations).useState();
}

applyPreset({ from, to, diffFrom, diffTo, label }: Preset) {
this.setDiffRange(diffFrom, diffTo);

this.state.$timeRange.setState(buildTimeRange(from, to));

this.updateTitle(label);
}

setDiffRange(diffFrom: string, diffTo: string) {
const $diffTimeRange = this.state.timeseriesPanel.state.body.state.$timeRange as SceneTimeRangeWithAnnotations;
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Should pull the state up?

Drilling down the state looks a bit cumbersome and makes testing harder do to mocking. Do you think it would make sense / be feasible to pull timeRange state up and save it only in SceneComparePanel. Timeseries panel looks for nearest timeRange anyway so will use the same one. This is not needed in this PR, just food for thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As we discussed face to face: this time range is actually a timerange-like object (a hack) that we use to enable annotations on the time series (see SceneTimeRangeWithAnnotations). So it makes sense that we keep it as close as possible as where it's used (it's also responsible to change the data received from the API on-the-fly, in order to add the annotation on the time series).

Also, if in the future the platform provides a way to add range selections out-of-the-box (via a new API or so), it's likely that it's going to be configured on the timeseries panel.

Finally, for testing, I wouldn't mock anything ; I'd use end-to-end testing as we do now because these user interactions are quite complex (because they involve many different components).


$diffTimeRange.setAnnotationTimeRange($diffTimeRange.buildAnnotationTimeRange(diffFrom, diffTo), true);
}

/**
* This function is responsible for automatically selecting half of the time range (from the time picker) that will be used to build the diff flame graph
* For the baseline panel, the leftmost part, for the comparison one, the rightmost part.
* In the future, we might want to be smarter and provides a way to select (e.g.) the region with the lowest resource consumption on the baseline panel vs
* the region with the highest consumption on the comparison panel.
*/
autoSelectDiffRange(selectWholeRange: boolean) {
const { $timeRange, target } = this.state;
const { from, to } = $timeRange.state.value;

if (selectWholeRange) {
this.setDiffRange(from.toISOString(), to.toISOString());
return;
}

const diff = to.diff(from);
const half = Math.round(diff / 2); // TODO: cap the max value?

// we have to create a new instance because add() mutates the original one
const middle = dateTime(from).add(half).toISOString();

if (target === CompareTarget.BASELINE) {
this.setDiffRange(from.toISOString(), middle);
} else {
this.setDiffRange(middle, to.toISOString());
}
}

updateTitle(label = '') {
const title = this.state.target === CompareTarget.BASELINE ? 'Baseline' : 'Comparison';
const newTitle = label ? `${title} (${label})` : title;

this.setState({ title: newTitle });
}

public static Component = ({ model }: SceneComponentProps<SceneComparePanel>) => {
const styles = useStyles2(getStyles);
const { target, title, timeseriesPanel: timeseries, timePicker, refreshPicker, filterKey } = model.useState();
const {
target,
color,
title,
timeseriesPanel: timeseries,
timePicker,
refreshPicker,
filterKey,
} = model.useState();
const styles = useStyles2(getStyles, color);

const filtersVariable = sceneGraph.findByKey(model, filterKey) as FiltersVariable;

return (
<div className={styles.panel} data-testid={`panel-${target}`}>
<div className={styles.panelHeader}>
<h6>{title}</h6>
<h6>
<div className={styles.colorCircle} />
{title}
</h6>

<div className={styles.timePicker}>
<timePicker.Component model={timePicker} />
Expand All @@ -238,7 +319,7 @@ export class SceneComparePanel extends SceneObjectBase<SceneComparePanelState> {
};
}

const getStyles = (theme: GrafanaTheme2) => ({
const getStyles = (theme: GrafanaTheme2, color: string) => ({
panel: css`
background-color: ${theme.colors.background.primary};
padding: ${theme.spacing(1)} ${theme.spacing(1)} 0 ${theme.spacing(1)};
Expand All @@ -252,11 +333,20 @@ const getStyles = (theme: GrafanaTheme2) => ({
margin-bottom: ${theme.spacing(2)};

& > h6 {
font-size: 15px;
height: 32px;
line-height: 32px;
margin: 0 ${theme.spacing(1)} 0 0;
}
`,
colorCircle: css`
display: inline-block;
background-color: ${color};
border-radius: 50%;
width: 9px;
height: 9px;
margin-right: 6px;
`,
timePicker: css`
display: flex;
justify-content: flex-end;
Expand All @@ -272,5 +362,9 @@ const getStyles = (theme: GrafanaTheme2) => ({
& [data-viz-panel-key] > * {
border: 0 none;
}

& [data-viz-panel-key] [data-testid='uplot-main-div'] {
cursor: crosshair;
}
`,
});
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { dateTime, LoadingState, TimeRange } from '@grafana/data';
import { DateTime, dateTime, LoadingState, TimeRange } from '@grafana/data';
import {
sceneGraph,
SceneObjectBase,
Expand Down Expand Up @@ -140,7 +140,7 @@ export class SceneTimeRangeWithAnnotations
timeEnd: annotationTimeRange.to.unix() * 1000,
});

// tradeoff: this will trigger any $data subscribers even though the data itself hasn't changed
// tradeoff: this will notify all the $data subscribers even though the data itself hasn't changed
$data?.setState({
data: {
...data,
Expand All @@ -149,8 +149,16 @@ export class SceneTimeRangeWithAnnotations
});
}

setAnnotationTimeRange(annotationTimeRange: TimeRange, updateTimeseries = false) {
this.setState({ annotationTimeRange });

if (updateTimeseries) {
this.updateTimeseriesAnnotation();
}
}

nullifyAnnotationTimeRange() {
this.setState({ annotationTimeRange: TIMERANGE_NIL });
this.setAnnotationTimeRange(TIMERANGE_NIL);
}

getUrlState() {
Expand All @@ -177,15 +185,22 @@ export class SceneTimeRangeWithAnnotations

const { annotationTimeRange } = this.state;

this.setState({
annotationTimeRange: evaluateTimeRange(
this.setAnnotationTimeRange(
this.buildAnnotationTimeRange(
parseUrlParam(diffFrom) ?? annotationTimeRange.from,
parseUrlParam(diffTo) ?? annotationTimeRange.to,
this.getTimeZone(),
this.state.fiscalYearStartMonth,
this.state.UNSAFE_nowDelay
),
});
parseUrlParam(diffTo) ?? annotationTimeRange.to
)
);
}

buildAnnotationTimeRange(diffFrom: string | DateTime, diffTo: string | DateTime) {
return evaluateTimeRange(
diffFrom,
diffTo,
this.getTimeZone(),
this.state.fiscalYearStartMonth,
this.state.UNSAFE_nowDelay
);
}

onTimeRangeChange(timeRange: TimeRange): void {
Expand All @@ -198,9 +213,7 @@ export class SceneTimeRangeWithAnnotations

// this triggers a timeseries request to the API
// TODO: caching?
this.setState({ annotationTimeRange: timeRange });

this.updateTimeseriesAnnotation();
this.setAnnotationTimeRange(timeRange, true);
}

onTimeZoneChange(timeZone: string): void {
Expand Down
Loading
Loading