From 83451683d131671771b0e97e052068b08bfe35bd Mon Sep 17 00:00:00 2001 From: maksim hodasevich <47758224+dogfrogfog@users.noreply.github.com> Date: Thu, 14 Jul 2022 01:02:08 +0200 Subject: [PATCH] feat: add new tooltip design (#1246) * add new tooltip design * fix jest tests * Update packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.tsx * makes it use the same mono font everywhere, some stylistic changes Co-authored-by: Ryan Perry Co-authored-by: Dmitry Filimonov --- .gitignore | 1 + cypress/integration/webapp/basic.ts | 32 +- .../FlameGraphComponent/Tooltip.module.scss | 64 +++- .../FlameGraphComponent/Tooltip.spec.tsx | 339 +++++++++--------- .../FlameGraphComponent/Tooltip.tsx | 216 ++++++++--- webapp/sass/components/tagsbar.scss | 4 +- webapp/sass/variables.css | 4 + 7 files changed, 413 insertions(+), 247 deletions(-) diff --git a/.gitignore b/.gitignore index ff37bb1c96..c2d9213b0c 100644 --- a/.gitignore +++ b/.gitignore @@ -25,6 +25,7 @@ benchmark/fixtures cypress/snapshots/**/__diff_output__ cypress/videos cypress/screenshots +coverage # this file may contain sensitive credentials pkg/server/testdata/oauth-config.yml dist diff --git a/cypress/integration/webapp/basic.ts b/cypress/integration/webapp/basic.ts index f8d9fdf52c..b317794ee7 100644 --- a/cypress/integration/webapp/basic.ts +++ b/cypress/integration/webapp/basic.ts @@ -169,13 +169,13 @@ describe('basic test', () => { cy.findByTestId('flamegraph-tooltip').should('not.be.visible'); - cy.waitForFlamegraphToRender().trigger('mousemove', 0, 0); + cy.waitForFlamegraphToRender().trigger('mousemove'); cy.findByTestId('flamegraph-tooltip').should('be.visible'); cy.findByTestId('flamegraph-tooltip-title').should('have.text', 'total'); - cy.findByTestId('flamegraph-tooltip-body').should( + cy.findByTestId('flamegraph-tooltip-table').should( 'have.text', - '100%, 988 samples, 9.88 seconds' + 'Share of CPU:100%CPU Time:9.88 secondsSamples:988' ); cy.waitForFlamegraphToRender().trigger('mouseout'); @@ -212,7 +212,7 @@ describe('basic test', () => { .findByTestId('flamegraph-tooltip') .should('not.be.visible'); - findFlamegraph(1).waitForFlamegraphToRender().trigger('mousemove', 0, 0); + findFlamegraph(1).waitForFlamegraphToRender().trigger('mousemove'); findFlamegraph(1).findByTestId('flamegraph-tooltip').should('be.visible'); @@ -220,10 +220,13 @@ describe('basic test', () => { .findByTestId('flamegraph-tooltip-title') .should('have.text', 'total'); findFlamegraph(1) - .findByTestId('flamegraph-tooltip-body') - .should('have.text', '100%, 991 samples, 9.91 seconds'); + .findByTestId('flamegraph-tooltip-table') + .should( + 'have.text', + 'Share of CPU:100%CPU Time:9.91 secondsSamples:991' + ); - findFlamegraph(1).waitForFlamegraphToRender().trigger('mousemove', 0, 0); + findFlamegraph(1).waitForFlamegraphToRender().trigger('mousemove'); findFlamegraph(1).waitForFlamegraphToRender().trigger('mouseout'); findFlamegraph(1) @@ -246,8 +249,11 @@ describe('basic test', () => { .findByTestId('flamegraph-tooltip-title') .should('have.text', 'total'); findFlamegraph(2) - .findByTestId('flamegraph-tooltip-body') - .should('have.text', '100%, 988 samples, 9.88 seconds'); + .findByTestId('flamegraph-tooltip-table') + .should( + 'have.text', + 'Share of CPU:100%CPU Time:9.88 secondsSamples:988' + ); findFlamegraph(2) .waitForFlamegraphToRender() @@ -281,13 +287,9 @@ describe('basic test', () => { cy.findByTestId('flamegraph-tooltip').should('be.visible'); cy.findByTestId('flamegraph-tooltip-title').should('have.text', 'total'); - cy.findByTestId('flamegraph-tooltip-left').should( - 'have.text', - 'Baseline: 991 samples, 9.91 seconds (100%)' - ); - cy.findByTestId('flamegraph-tooltip-right').should( + cy.findByTestId('flamegraph-tooltip-table').should( 'have.text', - 'Comparison: 987 samples, 9.87 seconds (100%)' + 'BaselineComparisonDiffShare of CPU:100%100%CPU Time:9.91 seconds9.87 secondsSamples:991987' ); }); }); diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.module.scss b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.module.scss index deff3bb0ba..4c7238e781 100644 --- a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.module.scss +++ b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.module.scss @@ -1,21 +1,59 @@ .flamegraphTooltip { - max-width: 80%; - + width: 420px; position: fixed; - background: #ffffff; - white-space: nowrap; + background: var(--ps-neutral-2); box-shadow: 1px 2px 4px 0px rgba(0, 0, 0, 0.3); border-radius: 4px; - padding: 3px 5px; - color: #333; + color: var(--ps-neutral-1); font-size: 12px; visibility: hidden; - z-index: 1; -} + z-index: 2; + + &.flamegraphDiffTooltip { + width: 450px; + } + + .flamegraphTooltipName { + color: var(--ps-neutral-1); + background-color: var(--ps-tooltip-header-bg); + border-top-right-radius: 4px; + border-top-left-radius: 4px; + font-weight: bold; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; + padding: 8px 10px; + font-family: SFMono-Regular, Consolas, Liberation Mono, Menlo, monospace; + } + + .functionName { + padding: 8px 10px 0; + font-family: SFMono-Regular, Consolas, Liberation Mono, Menlo, monospace; + + white-space: break-spaces; + word-break: break-all; + } + + .tooltipTable { + width: calc(100% - 10px * 2); + margin: 10px 10px 16px; + + th, + td { + border: 1px solid var(--ps-tooltip-header-bg); + padding: 5px; + } + + td { + width: 50%; + } -.flamegraphTooltipName { - font-weight: 500; - white-space: nowrap; - overflow: hidden; - text-overflow: ellipsis; + &.tooltipDiffTable { + th, + td { + width: 25%; + font-weight: normal; + } + } + } } diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.spec.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.spec.tsx index 2690cfd532..b6b18a4331 100644 --- a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.spec.tsx +++ b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.spec.tsx @@ -3,6 +3,7 @@ import React from 'react'; import { render, screen } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { Maybe } from 'true-myth'; +import type { Units } from '@pyroscope/models'; import { diffColorRed, diffColorGreen } from './color'; import Tooltip, { TooltipProps } from './Tooltip'; @@ -24,6 +25,53 @@ function TestCanvas(props: Omit) { } describe('Tooltip', () => { + function executeTooltipTest( + props: Omit, + expectedData: { + diff?: { text: string; color: string }; + title: string; + percent: string | number; + samples: string; + formattedValue: string; + } + ) { + render(); + + // since we are mocking the result + // we don't care exactly where it's hovered + userEvent.hover(screen.getByTestId('canvas')); + + expect(screen.getByTestId('flamegraph-tooltip-title')).toHaveTextContent( + expectedData.title + ); + expect( + screen.getByTestId('flamegraph-tooltip-function-name') + ).toHaveTextContent(expectedData.title); + + const tableComponent = screen.getByTestId('flamegraph-tooltip-table'); + expect(tableComponent).toContainHTML('table'); + + if (expectedData?.diff) { + expect(tableComponent).toContainHTML('thead'); + expect(tableComponent).toHaveTextContent('BaselineComparisonDiff'); + + const diffComponent = screen.getByTestId('flamegraph-tooltip-diff'); + expect(diffComponent).toHaveStyle({ color: expectedData.diff.color }); + expect(diffComponent).toHaveTextContent(expectedData.diff.text); + } + + const tableHeader = expectedData?.diff ? 'BaselineComparisonDiff' : ''; + const diff = expectedData?.diff ? expectedData.diff.text : ''; + + expect(tableComponent).toHaveTextContent( + tableHeader + + expectedData.percent + + diff + + expectedData.formattedValue + + expectedData.samples + ); + } + describe('"single" mode', () => { it('renders correctly', () => { const xyToData = (x: number, y: number) => @@ -33,64 +81,28 @@ describe('Tooltip', () => { total: 10, }); - render( - - ); - - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - expect(screen.getByTestId('flamegraph-tooltip-title')).toHaveTextContent( - 'function_title' - ); - expect(screen.getByTestId('flamegraph-tooltip-body')).toHaveTextContent( - '10%, 10 samples, 0.10 seconds' - ); + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData, + leftTicks: 100, + rightTicks: 100, + format: 'single' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:10%', + samples: 'Samples:10', + formattedValue: 'CPU Time:0.10 seconds', + title: 'function_title', + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); }); describe('"double" mode', () => { - function assertTooltipContent({ - title, - diffColor, - left, - right, - }: { - title: string; - diffColor: typeof diffColorRed | undefined; - left: string; - right: string; - }) { - expect(screen.getByTestId('flamegraph-tooltip-title')).toHaveTextContent( - title - ); - - if (diffColor) { - expect(screen.getByTestId('flamegraph-tooltip-title-diff')).toHaveStyle( - { - color: diffColor, - } - ); - } - - expect(screen.getByTestId('flamegraph-tooltip-left')).toHaveTextContent( - left - ); - expect(screen.getByTestId('flamegraph-tooltip-right')).toHaveTextContent( - right - ); - } - it("works with a function that hasn't changed", () => { const myxyToData = (x: number, y: number) => Maybe.of({ @@ -101,29 +113,28 @@ describe('Tooltip', () => { barTotal: 100, }); - render( - - ); - - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - assertTooltipContent({ + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData: myxyToData, + leftTicks: 100010, + rightTicks: 100, + format: 'double' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:0.1%100%', + formattedValue: 'CPU Time:1.00 second1.00 second', + samples: 'Samples:100100', title: 'my_function', - diffColor: undefined, - left: 'Baseline: 100 samples, 1.00 second (10%)', - right: 'Comparison: 100 samples, 1.00 second (10%)', - }); + diff: { + text: '(+99900.00%)', + color: 'rgb(200, 0, 0)', + }, + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); it('works with a function that has been added', () => { @@ -136,28 +147,28 @@ describe('Tooltip', () => { barTotal: 100, }); - render( - - ); - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - assertTooltipContent({ - title: 'my_function (new)', - diffColor: diffColorRed, - left: 'Baseline: 0 samples, < 0.01 seconds (0%)', - right: 'Comparison: 100 samples, 1.00 second (10%)', - }); + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData: myxyToData, + leftTicks: 1000, + rightTicks: 1000, + format: 'double' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:0%10%', + formattedValue: 'CPU Time:< 0.01 seconds1.00 second', + samples: 'Samples:0100', + title: 'my_function', + diff: { + text: '(new)', + color: 'rgb(200, 0, 0)', + }, + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); it('works with a function that has been removed', () => { @@ -170,28 +181,28 @@ describe('Tooltip', () => { barTotal: 100, }); - render( - - ); - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - assertTooltipContent({ - title: 'my_function (removed)', - diffColor: diffColorGreen, - left: 'Baseline: 100 samples, 1.00 second (10%)', - right: 'Comparison: 0 samples, < 0.01 seconds (0%)', - }); + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData: myxyToData, + leftTicks: 1000, + rightTicks: 1000, + format: 'double' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:10%0%', + formattedValue: 'CPU Time:1.00 second< 0.01 seconds', + samples: 'Samples:1000', + title: 'my_function', + diff: { + text: '(removed)', + color: 'rgb(0, 170, 0)', + }, + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); it('works with a function that became slower', () => { @@ -204,28 +215,28 @@ describe('Tooltip', () => { barTotal: 100, }); - render( - - ); - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - assertTooltipContent({ - title: 'my_function (+100.00%)', - diffColor: diffColorRed, - left: 'Baseline: 100 samples, 1.00 second (10%)', - right: 'Comparison: 200 samples, 2.00 seconds (20%)', - }); + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData: myxyToData, + leftTicks: 1000, + rightTicks: 1000, + format: 'double' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:10%20%', + formattedValue: 'CPU Time:1.00 second2.00 seconds', + samples: 'Samples:100200', + title: 'my_function', + diff: { + text: '(+100.00%)', + color: 'rgb(200, 0, 0)', + }, + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); it('works with a function that became faster', () => { @@ -238,28 +249,28 @@ describe('Tooltip', () => { barTotal: 100, }); - render( - - ); - // since we are mocking the result - // we don't care exactly where it's hovered - userEvent.hover(screen.getByTestId('canvas')); - - assertTooltipContent({ - title: 'my_function (-50.00%)', - diffColor: diffColorGreen, - left: 'Baseline: 200 samples, 2.00 seconds (20%)', - right: 'Comparison: 100 samples, 1.00 second (10%)', - }); + const tooltipProps = { + numTicks: 100, + sampleRate: 100, + xyToData: myxyToData, + leftTicks: 1000, + rightTicks: 1000, + format: 'double' as const, + units: 'samples' as Units, + }; + + const expectedTableData = { + percent: 'Share of CPU:20%10%', + formattedValue: 'CPU Time:2.00 seconds1.00 second', + samples: 'Samples:200100', + title: 'my_function', + diff: { + text: '(-50.00%)', + color: 'rgb(0, 170, 0)', + }, + }; + + executeTooltipTest(tooltipProps, expectedTableData); }); }); }); diff --git a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.tsx b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.tsx index c892b6bac0..32bba0d467 100644 --- a/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.tsx +++ b/packages/pyroscope-flamegraph/src/FlameGraph/FlameGraphComponent/Tooltip.tsx @@ -1,6 +1,7 @@ import React from 'react'; import { Maybe } from 'true-myth'; -import { Units } from '@pyroscope/models/src'; +import clsx from 'clsx'; +import type { Units } from '@pyroscope/models/src'; import type { Unwrapped } from 'true-myth/maybe'; import { getFormatter, @@ -48,6 +49,51 @@ export type TooltipProps = { } ); +const tooltipTitles: Record< + Units, + { percent: string; formattedValue: string } +> = { + objects: { + percent: '% of objects in RAM', + formattedValue: 'RAM amount', + }, + goroutines: { + percent: '% of goroutines', + formattedValue: 'goroutines', + }, + bytes: { + percent: '% of RAM', + formattedValue: 'bytes', + }, + samples: { + percent: 'Share of CPU', + formattedValue: 'CPU Time', + }, + lock_nanoseconds: { + percent: '% of Time spent', + formattedValue: 'seconds', + }, + lock_samples: { + percent: '% of contended locks', + formattedValue: 'locks', + }, + trace_samples: { + percent: '% of time', + formattedValue: 'samples', + }, + '': { + percent: '', + formattedValue: '', + }, +}; + +type TooltipData = { + units: Units; + percent: string | number; + samples: string; + formattedValue: string; +}; + export default function Tooltip(props: TooltipProps) { const { format, canvasRef, xyToData } = props; const [content, setContent] = React.useState({ @@ -58,8 +104,7 @@ export default function Tooltip(props: TooltipProps) { color: '', }, }, - left: '', - right: '', + tooltipData: [] as TooltipData[], }); const [style, setStyle] = React.useState(); @@ -109,13 +154,13 @@ export default function Tooltip(props: TooltipProps) { // set the content switch (data.format) { case 'single': { - const d = formatSingle( - formatter, - data.total, - sampleRate, - numTicks, - units - ); + const newLeftContent: TooltipData = { + percent: formatPercent(data.total / numTicks), + samples: + units === 'trace_samples' ? '' : numberWithCommas(data.total), + units, + formattedValue: formatter.format(data.total, sampleRate), + }; setContent({ title: { @@ -125,8 +170,7 @@ export default function Tooltip(props: TooltipProps) { color: '', }, }, - left: d.left, - right: '', + tooltipData: [newLeftContent], }); break; @@ -148,14 +192,14 @@ export default function Tooltip(props: TooltipProps) { totalRight: data.totalRight, rightTicks, title: data.name, + units, }, palette ); setContent({ title: d.title, - left: d.left, - right: d.right, + tooltipData: d.tooltipData, }); break; @@ -195,52 +239,111 @@ export default function Tooltip(props: TooltipProps) {
1, + })} style={style} ref={tooltipEl} >
{content.title.text} - - {`${content.title.diff.text.length > 0 ? ' ' : ''}${ - content.title.diff.text - }`} -
-
-
{content.left}
-
{content.right}
+
+ {content.title.text}
+ + {content.title.diff.text.length > 0 ? ( + + ) : ( + + )}
); } -interface Formatter { - format(samples: number, sampleRate: number): string; -} +function TooltipTable({ + data, + diff, +}: { + data: TooltipData[]; + diff?: { text: string; color: string }; +}) { + const [baselineData, comparisonData] = data; + + if (!baselineData) { + return null; + } -function formatSingle( - formatter: Formatter, - total: number, - sampleRate: number, - numTicks: number, - units: Units -) { - const percent = formatPercent(total / numTicks); - const samples = `${numberWithCommas(total)} samples,`; - const left = `${percent}, ${ - units === 'trace_samples' ? '' : samples - } ${formatter.format(total, sampleRate)}`; + return ( + + {comparisonData && ( + + + + + + + + )} + + + + + {comparisonData && ( + <> + + + + )} + + + + + {comparisonData && ( + <> + + + + + + {comparisonData && ( + <> + + + +
+ BaselineComparisonDiff
{tooltipTitles[baselineData.units].percent}:{baselineData.percent}{comparisonData.percent} + {diff && ( + + {diff.text} + + )} +
{tooltipTitles[baselineData.units].formattedValue}:{baselineData.formattedValue}{comparisonData.formattedValue} + + )} +
Samples:{baselineData.samples}{comparisonData.samples} + + )} +
+ ); +} - return { - left, - }; +interface Formatter { + format(samples: number, sampleRate: number): string; } function formatDouble( @@ -252,6 +355,7 @@ function formatDouble( totalRight, rightTicks, title, + units, }: { formatter: Formatter; sampleRate: number; @@ -260,6 +364,7 @@ function formatDouble( totalRight: number; rightTicks: number; title: string; + units: Units; }, palette: FlamegraphPalette = DefaultPalette ) { @@ -269,13 +374,19 @@ function formatDouble( const leftPercent = ratioToPercent(leftRatio); const rightPercent = ratioToPercent(rightRatio); - const left = `Baseline: ${numberWithCommas( - totalLeft - )} samples, ${formatter.format(totalLeft, sampleRate)} (${leftPercent}%)`; + const newLeft: TooltipData = { + percent: leftPercent + '%', + samples: numberWithCommas(totalLeft), + units, + formattedValue: formatter.format(totalLeft, sampleRate), + }; - const right = `Comparison: ${numberWithCommas( - totalRight - )} samples, ${formatter.format(totalRight, sampleRate)} (${rightPercent}%)`; + const newRight: TooltipData = { + percent: rightPercent + '%', + samples: numberWithCommas(totalRight), + units, + formattedValue: formatter.format(totalRight, sampleRate), + }; const totalDiff = percentDiff(leftPercent, rightPercent); @@ -307,8 +418,7 @@ function formatDouble( color: tooltipDiffColor, }, }, - left, - right, + tooltipData: [newLeft, newRight], }; } diff --git a/webapp/sass/components/tagsbar.scss b/webapp/sass/components/tagsbar.scss index d2a7679923..7ad4e89406 100644 --- a/webapp/sass/components/tagsbar.scss +++ b/webapp/sass/components/tagsbar.scss @@ -33,7 +33,7 @@ caret-color: var(--ps-neutral-2); color: var(--ps-grey-primary); letter-spacing: 0px; - font-family: monospace; + font-family: SFMono-Regular, Consolas, Liberation Mono, Menlo, monospace; font-size: 16px; border: 1px solid var(--ps-ui-border); padding: 0.25em 0.375em; @@ -79,7 +79,7 @@ code { font-size: 16px; - font-family: monospace; + font-family: SFMono-Regular, Consolas, Liberation Mono, Menlo, monospace; background: none; border: 1px solid transparent; box-shadow: none; diff --git a/webapp/sass/variables.css b/webapp/sass/variables.css index 43953ccb5d..80b1b700d6 100644 --- a/webapp/sass/variables.css +++ b/webapp/sass/variables.css @@ -63,6 +63,8 @@ --ps-immutable-linked-border: #eb7b18; /* linked border color */ --ps-immutable-white: #ffffff; /* white: Use only when it should be white regardless of color mode */ --ps-immutable-placeholder-text: #333333; /* placeholder text color */ + + --ps-tooltip-header-bg: #d8d8d8; /* flamegraph tooltip heeader */ } [data-theme='light'], @@ -94,4 +96,6 @@ --ps-green-primary: #0bdb79; /* standard green */ --ps-green-highlight: #5bdc9e; /* highlight (hover) green */ --ps-green-disabled: #00a757; /* disabled green */ + + --ps-tooltip-header-bg: #d8d8d8; /* flamegraph tooltip heeader */ }