Skip to content

ref(perf): Don't use 'mark.<vital>' for drawing web vitals in event details #38023

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

Merged
merged 3 commits into from
Aug 19, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
Expand Up @@ -22,6 +22,11 @@ type Props = {
generateBounds: (bounds: SpanBoundsType) => SpanGeneratedBoundsType;
};

type VitalLabel = {
isPoorValue: boolean;
vital: Vital;
};

function MeasurementsPanel(props: Props) {
const {event, generateBounds, dividerPosition} = props;
const measurements = getMeasurements(event, generateBounds);
Expand All @@ -44,30 +49,26 @@ function MeasurementsPanel(props: Props) {
return null;
}

// Measurements are referred to by their full name `measurements.<name>`
// here but are stored using their abbreviated name `<name>`. Make sure
// to convert it appropriately.
const vitals: Vital[] = Object.keys(verticalMark.marks).map(
name => WEB_VITAL_DETAILS[`measurements.${name}`]
);
const vitalLabels: VitalLabel[] = Object.keys(verticalMark.marks).map(name => ({
vital: WEB_VITAL_DETAILS[`measurements.${name}`],
isPoorValue: verticalMark.marks[name].failedThreshold,
}));

if (vitals.length > 1) {
if (vitalLabels.length > 1) {
return (
<MultiLabelContainer
key={String(timestamp)}
failedThreshold={verticalMark.failedThreshold}
left={toPercent(bounds.left || 0)}
vitals={vitals}
vitalLabels={vitalLabels}
/>
);
}

return (
<LabelContainer
key={String(timestamp)}
failedThreshold={verticalMark.failedThreshold}
left={toPercent(bounds.left || 0)}
vital={vitals[0]}
vitalLabel={vitalLabels[0]}
/>
);
})}
Expand Down Expand Up @@ -123,9 +124,8 @@ const Label = styled('div')<{
export default MeasurementsPanel;

type LabelContainerProps = {
failedThreshold: boolean;
left: string;
vital: Vital;
vitalLabel: VitalLabel;
};

type LabelContainerState = {
Expand All @@ -149,7 +149,7 @@ class LabelContainer extends Component<LabelContainerProps> {
elementDOMRef = createRef<HTMLDivElement>();

render() {
const {left, failedThreshold, vital} = this.props;
const {left, vitalLabel} = this.props;

return (
<StyledLabelContainer
Expand All @@ -158,18 +158,23 @@ class LabelContainer extends Component<LabelContainerProps> {
left: `clamp(calc(0.5 * ${this.state.width}px), ${left}, calc(100% - 0.5 * ${this.state.width}px))`,
}}
>
<Label failedThreshold={failedThreshold} isSingleLabel>
<Tooltip title={vital.name} position="top" containerDisplayMode="inline-block">
{vital.acronym}
<Label failedThreshold={vitalLabel.isPoorValue} isSingleLabel>
<Tooltip
title={vitalLabel.vital.name}
position="top"
containerDisplayMode="inline-block"
>
{vitalLabel.vital.acronym}
</Tooltip>
</Label>
</StyledLabelContainer>
);
}
}

type MultiLabelContainerProps = Omit<LabelContainerProps, 'vital'> & {
vitals: Vital[];
type MultiLabelContainerProps = {
left: string;
vitalLabels: VitalLabel[];
};

class MultiLabelContainer extends Component<MultiLabelContainerProps> {
Expand All @@ -190,7 +195,7 @@ class MultiLabelContainer extends Component<MultiLabelContainerProps> {
elementDOMRef = createRef<HTMLDivElement>();

render() {
const {left, failedThreshold, vitals} = this.props;
const {left, vitalLabels} = this.props;

return (
<StyledMultiLabelContainer
Expand All @@ -199,14 +204,14 @@ class MultiLabelContainer extends Component<MultiLabelContainerProps> {
left: `clamp(calc(0.5 * ${this.state.width}px), ${left}, calc(100% - 0.5 * ${this.state.width}px))`,
}}
>
{vitals.map(vital => (
<Label failedThreshold={failedThreshold} key={`${vital.name}-label`}>
{vitalLabels.map(label => (
<Label failedThreshold={label.isPoorValue} key={`${label.vital.name}-label`}>
<Tooltip
title={vital.name}
title={label.vital.name}
position="top"
containerDisplayMode="inline-block"
>
{vital.acronym}
{label.vital.acronym}
</Tooltip>
</Label>
))}
Expand Down
37 changes: 30 additions & 7 deletions static/app/components/events/interfaces/spans/utils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {Organization} from 'sentry/types';
import {EntryType, EventTransaction} from 'sentry/types/event';
import {assert} from 'sentry/types/utils';
import trackAdvancedAnalyticsEvent from 'sentry/utils/analytics/trackAdvancedAnalyticsEvent';
import {WebVital} from 'sentry/utils/fields';
import {TraceError} from 'sentry/utils/performance/quickTrace/types';
import {WEB_VITAL_DETAILS} from 'sentry/utils/performance/vitals/constants';
import {getPerformanceTransaction} from 'sentry/utils/performanceForSentry';
Expand Down Expand Up @@ -512,6 +513,7 @@ export const durationlessBrowserOps = ['mark', 'paint'];

type Measurements = {
[name: string]: {
failedThreshold: boolean;
timestamp: number;
value: number | undefined;
};
Expand Down Expand Up @@ -541,26 +543,37 @@ export function getMeasurements(
event: EventTransaction,
generateBounds: (bounds: SpanBoundsType) => SpanGeneratedBoundsType
): Map<number, VerticalMark> {
if (!event.measurements) {
if (!event.measurements || !event.startTimestamp) {
return new Map();
}

const {startTimestamp} = event;

// Note: CLS and INP should not be included here, since they are not timeline-based measurements.
const allowedVitals = new Set<string>([
WebVital.FCP,
WebVital.FP,
WebVital.FID,
WebVital.LCP,
WebVital.TTFB,
]);

const measurements = Object.keys(event.measurements)
.filter(name => name.startsWith('mark.'))
.filter(name => allowedVitals.has(`measurements.${name}`))
.map(name => {
const slug = name.slice('mark.'.length);
const associatedMeasurement = event.measurements![slug];
const associatedMeasurement = event.measurements![name];
return {
name,
timestamp: event.measurements![name].value,
// Time timestamp is in seconds, but the measurement value is given in ms so convert it here
timestamp: startTimestamp + associatedMeasurement.value / 1000,
value: associatedMeasurement ? associatedMeasurement.value : undefined,
};
});

const mergedMeasurements = new Map<number, VerticalMark>();

measurements.forEach(measurement => {
const name = measurement.name.slice('mark.'.length);
const name = measurement.name;
const value = measurement.value;

const bounds = generateBounds({
Expand All @@ -584,11 +597,14 @@ export function getMeasurements(
if (positionDelta <= MERGE_LABELS_THRESHOLD_PERCENT) {
const verticalMark = mergedMeasurements.get(otherPos)!;

const {poorThreshold} = WEB_VITAL_DETAILS[`measurements.${name}`];

verticalMark.marks = {
...verticalMark.marks,
[name]: {
value,
timestamp: measurement.timestamp,
failedThreshold: value ? value >= poorThreshold : false,
},
};

Expand All @@ -601,15 +617,22 @@ export function getMeasurements(
}
}

const {poorThreshold} = WEB_VITAL_DETAILS[`measurements.${name}`];

const marks = {
[name]: {value, timestamp: measurement.timestamp},
[name]: {
value,
timestamp: measurement.timestamp,
failedThreshold: value ? value >= poorThreshold : false,
},
};

mergedMeasurements.set(roundedPos, {
marks,
failedThreshold: hasFailedThreshold(marks),
});
});

return mergedMeasurements;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -734,9 +734,9 @@ describe('TraceView', () => {
generateSampleSpan('browser', 'test5', 'f000000000000000', 'a000000000000000', event);

event.measurements = {
'mark.fcp': {value: 1000},
'mark.fp': {value: 1050},
'mark.lcp': {value: 1100},
fcp: {value: 1000},
fp: {value: 1050},
lcp: {value: 1100},
};

const waterfallModel = new WaterfallModel(event);
Expand All @@ -758,9 +758,12 @@ describe('TraceView', () => {
const event = generateSampleEvent();
generateSampleSpan('browser', 'test1', 'b000000000000000', 'a000000000000000', event);

event.startTimestamp = 1;
event.endTimestamp = 100;

event.measurements = {
'mark.fcp': {value: 1000},
'mark.lcp': {value: 200000000},
fcp: {value: 858.3002090454102, unit: 'millisecond'},
lcp: {value: 1000363.800048828125, unit: 'millisecond'},
};

const waterfallModel = new WaterfallModel(event);
Expand Down