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(webapp): Issue when comparison / diff timelines are out of range #1615

Merged
merged 20 commits into from
Nov 1, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import React from 'react';
import Button from '@webapp/ui/Button';
import { TimelineData } from '@webapp/components/TimelineChart/TimelineChartWrapper';
import { useSync } from './useSync';
import StatusMessage from '@webapp/ui/StatusMessage';
import styles from './styles.module.scss';

interface SyncTimelinesProps {
timeline: TimelineData;
leftSelection: {
from: string;
to: string;
};
rightSelection: {
from: string;
to: string;
};
onSync: (from: string, until: string) => void;
}

function SyncTimelines({
timeline,
leftSelection,
rightSelection,
onSync,
}: SyncTimelinesProps) {
const { isWarningHidden, onIgnore, title, onSyncClick } = useSync({
timeline,
leftSelection,
rightSelection,
onSync,
});

if (isWarningHidden) {
return null;
}

return (
<StatusMessage
type="warning"
message={title}
rightSide={
<div className={styles.buttons}>
<Button
data-testid="sync-ignore-button"
onClick={onIgnore}
className={styles.ignoreButton}
>
Ignore
</Button>
<Button
data-testid="sync-button"
onClick={onSyncClick}
className={styles.syncButton}
>
Sync Timelines
</Button>
eh-am marked this conversation as resolved.
Show resolved Hide resolved
</div>
}
/>
);
}

export default SyncTimelines;
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
.buttons {
display: flex;
align-items: center;
flex-direction: row;
}

.syncButton {
border: 2px solid var(--ps-ui-border);
cursor: pointer;
padding: 3px 5px;
border-radius: 4px;
display: flex;
flex-shrink: 0;
margin-left: 5px;
background-color: transparent;

&:hover:not(:disabled) {
background-color: transparent;
}
}

.ignoreButton {
@extend .syncButton;

margin-left: 10px;
}
105 changes: 105 additions & 0 deletions webapp/javascript/components/TimelineChart/SyncTimelines/useSync.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import { useEffect, useState } from 'react';
import { centerTimelineData } from '@webapp/components/TimelineChart/centerTimelineData';
import { TimelineData } from '@webapp/components/TimelineChart/TimelineChartWrapper';
import { markingsFromSelection, Selection } from '../markings';

interface UseSyncParams {
timeline: TimelineData;
leftSelection: {
from: string;
to: string;
};
rightSelection: {
from: string;
to: string;
};
onSync: (from: string, until: string) => void;
}

const timeOffset = 5 * 60 * 1000;
const selectionOffset = 5000;

const getTitle = (leftInRange: boolean, rightInRange: boolean) => {
if (!leftInRange && !rightInRange) {
return 'Warning: Baseline and Comparison timeline selections are out of range';
}
if (!rightInRange) {
return 'Warning: Comparison timeline selection is out of range';
}
return 'Warning: Baseline timeline selection is out of range';
};

export function useSync({
timeline,
leftSelection,
rightSelection,
onSync,
}: UseSyncParams) {
const [isIgnoring, setIgnoring] = useState(false);

useEffect(() => {
if (isIgnoring) {
setIgnoring(false);
}
}, [leftSelection, rightSelection, timeline]);

const [
{
xaxis: { from: leftMarkingsFrom, to: leftMarkingsTo },
},
] = markingsFromSelection('single', {
...leftSelection,
} as Selection);
const [
{
xaxis: { from: rightMarkingsFrom, to: rightMarkingsTo },
},
] = markingsFromSelection('single', {
...rightSelection,
} as Selection);
Copy link
Collaborator

Choose a reason for hiding this comment

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

My understanding is that you are using this to convert an arbitrary from/to to a timestamp right? If so you can use https://github.com/pyroscope-io/pyroscope/blob/569091ed1a27cbe86611420b4fdf3bcdf3b330d6/webapp/javascript/util/formatDate.ts#L70-L89
not the best name i know

I am saying this bc markingsFromSelection is all about markings, those vertical lines in the timeline:
image.
Point is that that markingsFromSelection function is purely visual. Since you don't need that aspect, you can use formatAsOBject directly.


const centeredData = centerTimelineData(timeline);

const timelineFrom = centeredData?.[0]?.[0];
const timelineTo = centeredData?.[centeredData?.length - 1]?.[0];

const leftInRange =
leftMarkingsFrom + timeOffset >= timelineFrom &&
leftMarkingsTo - timeOffset <= timelineTo;

const rightInRange =
rightMarkingsFrom + timeOffset >= timelineFrom &&
rightMarkingsTo - timeOffset <= timelineTo;
pavelpashkovsky marked this conversation as resolved.
Show resolved Hide resolved

const onSyncClick = () => {
const selectionsLimits = [
leftMarkingsFrom,
leftMarkingsTo,
rightMarkingsFrom,
rightMarkingsTo,
];
const selectionMin = Math.min(...selectionsLimits);
const selectionMax = Math.max(...selectionsLimits);

const offset = [
leftSelection.from,
rightSelection.from,
leftSelection.to,
rightSelection.to,
].some((p) => String(p).startsWith('now'))
? selectionOffset
: 1;
pavelpashkovsky marked this conversation as resolved.
Show resolved Hide resolved

onSync(String(selectionMin - offset), String(selectionMax + offset));
};

return {
isWarningHidden:
!timeline.data?.samples.length ||
(leftInRange && rightInRange) ||
isIgnoring,
title: getTitle(leftInRange, rightInRange),
onIgnore: () => setIgnoring(true),
onSyncClick,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,15 @@ import Annotation from './Annotation';
import styles from './TimelineChartWrapper.module.css';
import { markingsFromAnnotations, markingsFromSelection } from './markings';
import { ContextMenuProps } from './ContextMenu.plugin';
import { centerTimelineData } from './centerTimelineData';

export interface TimelineGroupData {
data: Group;
tagName: string;
color?: Color;
}

interface TimelineData {
export interface TimelineData {
data?: Timeline;
color?: string;
}
Expand Down Expand Up @@ -521,28 +522,4 @@ function areTimelinesTheSame(
return smallest.samples.every((a) => map.has(a));
}

// Since profiling data is chuked by 10 seconds slices
// it's more user friendly to point a `center` of a data chunk
// as a bar rather than starting point, so we add 5 seconds to each chunk to 'center' it
function centerTimelineData(timelineData: TimelineData) {
return timelineData.data
? decodeTimelineData(timelineData.data).map((x) => [
x[0] + 5000,
x[1] === 0 ? 0 : x[1] - 1,
])
: [[]];
}

function decodeTimelineData(timeline: Timeline) {
if (!timeline) {
return [];
}
let time = timeline.startTime;
return timeline.samples.map((x) => {
const res = [time * 1000, x];
time += timeline.durationDelta;
return res;
});
}

export default TimelineChartWrapper;
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import type { Timeline } from '@webapp/models/timeline';

export interface TimelineData {
data?: Timeline;
color?: string;
}

function decodeTimelineData(timeline: Timeline) {
if (!timeline) {
return [];
}
let time = timeline.startTime;
return timeline.samples.map((x) => {
const res = [time * 1000, x];
time += timeline.durationDelta;
return res;
});
}

// Since profiling data is chuked by 10 seconds slices
// it's more user friendly to point a `center` of a data chunk
// as a bar rather than starting point, so we add 5 seconds to each chunk to 'center' it
export function centerTimelineData(timelineData: TimelineData) {
return timelineData.data
? decodeTimelineData(timelineData.data).map((x) => [
x[0] + 5000,
x[1] === 0 ? 0 : x[1] - 1,
])
: [[]];
}
2 changes: 1 addition & 1 deletion webapp/javascript/components/TimelineChart/markings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export function markingsFromAnnotations(
}

// Unify these types
interface Selection {
export interface Selection {
from: string;
to: string;
color: Color;
Expand Down
9 changes: 9 additions & 0 deletions webapp/javascript/pages/ContinuousComparisonView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
selectQueries,
} from '@webapp/redux/reducers/continuous';
import TimelineChartWrapper from '@webapp/components/TimelineChart/TimelineChartWrapper';
import SyncTimelines from '@webapp/components/TimelineChart/SyncTimelines';
import Toolbar from '@webapp/components/Toolbar';
import ExportData from '@webapp/components/ExportData';
import useExportToFlamegraphDotCom from '@webapp/components/exportToFlamegraphDotCom.hook';
Expand Down Expand Up @@ -129,6 +130,14 @@ function ComparisonApp() {
}
selectionType="double"
/>
<SyncTimelines
timeline={leftTimeline}
leftSelection={{ from: leftFrom, to: leftUntil }}
rightSelection={{ from: rightFrom, to: rightUntil }}
onSync={(from, until) => {
dispatch(actions.setFromAndUntil({ from, until }));
}}
/>
</LoadingOverlay>
</Box>
<div
Expand Down
9 changes: 9 additions & 0 deletions webapp/javascript/pages/ContinuousDiffView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import useTags from '@webapp/hooks/tags.hook';
import Toolbar from '@webapp/components/Toolbar';
import TagsBar from '@webapp/components/TagsBar';
import TimelineChartWrapper from '@webapp/components/TimelineChart/TimelineChartWrapper';
import SyncTimelines from '@webapp/components/TimelineChart/SyncTimelines';
import useExportToFlamegraphDotCom from '@webapp/components/exportToFlamegraphDotCom.hook';
import { LoadingOverlay } from '@webapp/ui/LoadingOverlay';
import ExportData from '@webapp/components/ExportData';
Expand Down Expand Up @@ -143,6 +144,14 @@ function ComparisonDiffApp() {
<TimelineTitle titleKey={diffView.profile?.metadata.units} />
}
/>
<SyncTimelines
timeline={leftTimeline}
leftSelection={{ from: leftFrom, to: leftUntil }}
rightSelection={{ from: rightFrom, to: rightUntil }}
onSync={(from, until) => {
dispatch(actions.setFromAndUntil({ from, until }));
}}
/>
</LoadingOverlay>
</Box>
<div className="diff-instructions-wrapper">
Expand Down
19 changes: 0 additions & 19 deletions webapp/javascript/ui/StatusMessage/StatusMessage.module.css

This file was deleted.

44 changes: 44 additions & 0 deletions webapp/javascript/ui/StatusMessage/StatusMessage.module.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
.statusMessage {
width: 100%;
display: flex;
flex-direction: row;
align-items: center;
justify-content: space-between;
}

.success {
color: var(--ps-neutral-2);
background-color: var(--ps-green-disabled);
padding: 10px;
margin: 1em 0;
padding: 1em;
}

.error {
color: var(--ps-neutral-2);
font-weight: bold;
background-color: var(--ps-red-primary);
margin: 1em 0;
padding: 1em;
}

.warning {
display: flex;
flex-direction: row;
justify-content: space-between;
align-items: center;
background-color: #f2cd51;
color: var(--ps-immutable-placeholder-text);
font-size: 12px;
border-radius: 4px;
border: 1px solid var(--ps-ui-border);
padding: 5px;
font-weight: bold;
line-height: 16px;
}

.rightSideWrapper {
&:empty {
display: none;
}
}
Loading