Skip to content

Commit ada4b5f

Browse files
authored
feat(tracemetrics): Do not allow deletion of metrics used in equations (#112893)
To avoid invalid states, disable deleting metrics that are used in equations.
1 parent 239b38a commit ada4b5f

File tree

8 files changed

+239
-11
lines changed

8 files changed

+239
-11
lines changed

static/app/views/explore/metrics/equationBuilder.tsx

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,13 @@ import {
99
} from 'sentry/components/arithmeticBuilder/token';
1010
import {tokenizeExpression} from 'sentry/components/arithmeticBuilder/tokenizer';
1111

12+
/**
13+
* Extracts the set of reference labels (e.g. ["A", "B"]) from an Expression's tokens.
14+
*/
15+
function extractReferenceLabels(expression: Expression): string[] {
16+
return expression.tokens.filter(isTokenReference).map(token => token.text);
17+
}
18+
1219
/**
1320
* Takes an expression and map of references and returns the internal string representation that uses the references.
1421
*/
@@ -72,9 +79,11 @@ export function EquationBuilder({
7279
expression,
7380
referenceMap,
7481
handleExpressionChange,
82+
onReferenceLabelsChange,
7583
}: {
7684
expression: string;
7785
handleExpressionChange: (expression: Expression) => void;
86+
onReferenceLabelsChange?: (labels: string[]) => void;
7887
referenceMap?: Record<string, string>;
7988
}) {
8089
const [_, startTransition] = useTransition();
@@ -113,6 +122,16 @@ export function EquationBuilder({
113122
}
114123
}, [referenceMap, internalExpression, handleExpressionChange]);
115124

125+
// Report which labels this equation references after unresolving.
126+
// Cleans up on unmount so deleted equations don't block metric deletion.
127+
useEffect(() => {
128+
const expr = new Expression(internalExpression, references);
129+
onReferenceLabelsChange?.(extractReferenceLabels(expr));
130+
return () => {
131+
onReferenceLabelsChange?.([]);
132+
};
133+
}, [internalExpression, references, onReferenceLabelsChange]);
134+
116135
const handleInternalExpressionChange = useCallback(
117136
(newExpression: Expression) => {
118137
startTransition(() => {
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
import {useCallback, useMemo, useState} from 'react';
2+
3+
/**
4+
* Tracks which metric labels (A, B, etc.) are referenced by equations.
5+
* Each EquationBuilder reports its labels via onEquationLabelsChange after
6+
* unresolving its expression. The aggregate set is derived from state.
7+
*/
8+
export function useEquationReferencedLabels() {
9+
const [equationLabels, onEquationLabelsChangeState] = useState(
10+
new Map<string, string[]>()
11+
);
12+
13+
const onEquationLabelsChange = useCallback(
14+
(equationLabel: string, labels: string[]) => {
15+
onEquationLabelsChangeState(prev => new Map(prev).set(equationLabel, labels));
16+
},
17+
[]
18+
);
19+
20+
const referencedMetricLabels = useMemo(() => {
21+
const set = new Set<string>();
22+
for (const labels of equationLabels.values()) {
23+
for (const label of labels) {
24+
set.add(label);
25+
}
26+
}
27+
return set;
28+
}, [equationLabels]);
29+
30+
return {referencedMetricLabels, onEquationLabelsChange};
31+
}

static/app/views/explore/metrics/metricPanel/index.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ interface MetricPanelProps extends React.HTMLAttributes<HTMLDivElement> {
4444
dragListeners?: SyntheticListenerMap;
4545
isAnyDragging?: boolean;
4646
isDragging?: boolean;
47+
onEquationLabelsChange?: (equationLabel: string, labels: string[]) => void;
4748
ref?: React.Ref<HTMLDivElement>;
4849
referenceMap?: Record<string, string>;
50+
referencedMetricLabels?: Set<string>;
4951
}
5052

5153
export function MetricPanel({
@@ -58,6 +60,8 @@ export function MetricPanel({
5860
isDragging,
5961
style,
6062
ref,
63+
referencedMetricLabels,
64+
onEquationLabelsChange,
6165
...rest
6266
}: MetricPanelProps) {
6367
const organization = useOrganization();
@@ -126,6 +130,8 @@ export function MetricPanel({
126130
queryLabel={queryLabel}
127131
referenceMap={referenceMap}
128132
dragListeners={dragListeners}
133+
referencedMetricLabels={referencedMetricLabels}
134+
onEquationLabelsChange={onEquationLabelsChange}
129135
/>
130136
</Container>
131137
{visualize.visible ? (

static/app/views/explore/metrics/metricPanel/sortableMetricPanel.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,9 @@ interface SortableMetricPanelProps {
1111
queryLabel: string;
1212
sortableId: string;
1313
traceMetric: TraceMetric;
14+
onEquationLabelsChange?: (equationLabel: string, labels: string[]) => void;
1415
referenceMap?: Record<string, string>;
16+
referencedMetricLabels?: Set<string>;
1517
}
1618

1719
export function SortableMetricPanel({
@@ -20,6 +22,8 @@ export function SortableMetricPanel({
2022
queryIndex,
2123
queryLabel,
2224
referenceMap,
25+
referencedMetricLabels,
26+
onEquationLabelsChange,
2327
isAnyDragging,
2428
canDrag,
2529
}: SortableMetricPanelProps) {
@@ -42,6 +46,8 @@ export function SortableMetricPanel({
4246
dragListeners={canDrag ? listeners : undefined}
4347
isAnyDragging={isAnyDragging}
4448
isDragging={isDragging}
49+
referencedMetricLabels={referencedMetricLabels}
50+
onEquationLabelsChange={onEquationLabelsChange}
4551
{...attributes}
4652
/>
4753
);

static/app/views/explore/metrics/metricToolbar/deleteMetricButton.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import {useOrganization} from 'sentry/utils/useOrganization';
66
import {canUseMetricsUIRefresh} from 'sentry/views/explore/metrics/metricsFlags';
77
import {useRemoveMetric} from 'sentry/views/explore/metrics/metricsQueryParams';
88

9-
export function DeleteMetricButton() {
9+
export function DeleteMetricButton({disabled}: {disabled?: boolean}) {
1010
const organization = useOrganization();
1111
const removeMetric = useRemoveMetric();
1212

@@ -17,6 +17,10 @@ export function DeleteMetricButton() {
1717
icon={<IconDelete />}
1818
size="zero"
1919
onClick={removeMetric}
20+
disabled={disabled}
21+
tooltipProps={{
22+
title: disabled ? t('This metric is used in an equation') : undefined,
23+
}}
2024
aria-label={t('Delete Metric')}
2125
/>
2226
);
@@ -28,6 +32,10 @@ export function DeleteMetricButton() {
2832
icon={<IconDelete />}
2933
aria-label={t('Delete metric')}
3034
onClick={removeMetric}
35+
disabled={disabled}
36+
tooltipProps={{
37+
title: disabled ? t('This metric is used in an equation') : undefined,
38+
}}
3139
/>
3240
);
3341
}

static/app/views/explore/metrics/metricToolbar/index.tsx

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,18 @@ interface MetricToolbarProps {
3232
queryLabel: string;
3333
traceMetric: TraceMetric;
3434
dragListeners?: SyntheticListenerMap;
35+
onEquationLabelsChange?: (equationLabel: string, labels: string[]) => void;
3536
referenceMap?: Record<string, string>;
37+
referencedMetricLabels?: Set<string>;
3638
}
3739

3840
export function MetricToolbar({
3941
traceMetric,
4042
queryLabel,
4143
referenceMap,
4244
dragListeners,
45+
referencedMetricLabels,
46+
onEquationLabelsChange,
4347
}: MetricToolbarProps) {
4448
const organization = useOrganization();
4549
const breakpoints = useBreakpoints();
@@ -58,6 +62,20 @@ export function MetricToolbar({
5862
metricQueries.filter(q => isVisualizeFunction(q.queryParams.visualizes[0]!)).length >
5963
1 || isVisualizeEquation(visualize);
6064

65+
// A metric function cannot be deleted if it is referenced by any equation.
66+
// referencedMetricLabels is precomputed from the stored equations and
67+
// overridden with exact labels when the user edits an equation, so that
68+
// duplicate metrics only block deletion of the specific label used.
69+
const isReferencedByEquation =
70+
isVisualizeFunction(visualize) && (referencedMetricLabels?.has(queryLabel) ?? false);
71+
72+
const handleReferenceLabelsChange = useCallback(
73+
(labels: string[]) => {
74+
onEquationLabelsChange?.(queryLabel, labels);
75+
},
76+
[onEquationLabelsChange, queryLabel]
77+
);
78+
6179
const handleExpressionChange = useCallback(
6280
(newExpression: Expression) => {
6381
setVisualize(visualize.replace({yAxis: `${EQUATION_PREFIX}${newExpression.text}`}));
@@ -115,9 +133,10 @@ export function MetricToolbar({
115133
expression={visualize.expression.text}
116134
referenceMap={referenceMap}
117135
handleExpressionChange={handleExpressionChange}
136+
onReferenceLabelsChange={handleReferenceLabelsChange}
118137
/>
119138
) : null}
120-
{canRemoveMetric && <DeleteMetricButton />}
139+
{canRemoveMetric && <DeleteMetricButton disabled={isReferencedByEquation} />}
121140
</Grid>
122141
{isNarrow && isVisualizeFunction(visualize) && (
123142
<Filter traceMetric={traceMetric} />
@@ -165,9 +184,10 @@ export function MetricToolbar({
165184
expression={visualize.expression.text}
166185
referenceMap={referenceMap}
167186
handleExpressionChange={handleExpressionChange}
187+
onReferenceLabelsChange={handleReferenceLabelsChange}
168188
/>
169189
) : null}
170-
{canRemoveMetric && <DeleteMetricButton />}
190+
{canRemoveMetric && <DeleteMetricButton disabled={isReferencedByEquation} />}
171191
</Grid>
172192
);
173193
}

static/app/views/explore/metrics/metricsTab.spec.tsx

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -731,4 +731,79 @@ describe('MetricsTabContent', () => {
731731
expect(screen.getByRole('button', {name: 'Add Metric'})).toBeDisabled();
732732
expect(screen.getByRole('button', {name: 'Add Equation'})).toBeDisabled();
733733
});
734+
735+
it('disables delete button for metrics referenced by an equation', async () => {
736+
const orgWithEquations = OrganizationFixture({
737+
features: ['tracemetrics-enabled', 'tracemetrics-equations-in-explore'],
738+
});
739+
740+
const metricA = JSON.stringify({
741+
metric: {name: 'metricA', type: 'distribution', unit: 'none'},
742+
query: '',
743+
aggregateFields: [{yAxes: ['sum(value,metricA,distribution,none)']}],
744+
aggregateSortBys: [],
745+
mode: 'samples',
746+
});
747+
748+
const metricB = JSON.stringify({
749+
metric: {name: 'metricB', type: 'distribution', unit: 'none'},
750+
query: '',
751+
aggregateFields: [{yAxes: ['sum(value,metricB,distribution,none)']}],
752+
aggregateSortBys: [],
753+
mode: 'samples',
754+
});
755+
756+
const equation = JSON.stringify({
757+
metric: {name: '', type: ''},
758+
query: '',
759+
aggregateFields: [{yAxes: ['equation|sum(value,metricA,distribution,none)']}],
760+
aggregateSortBys: [],
761+
mode: 'samples',
762+
});
763+
764+
render(
765+
<ProviderWrapper>
766+
<MetricsTabContent datePageFilterProps={datePageFilterProps} />
767+
</ProviderWrapper>,
768+
{
769+
organization: orgWithEquations,
770+
initialRouterConfig: {
771+
location: {
772+
pathname: '/organizations/:orgId/explore/metrics/',
773+
query: {
774+
start: '2025-04-10T14%3A37%3A55',
775+
end: '2025-04-10T20%3A04%3A51',
776+
metric: [metricA, metricB, equation],
777+
title: 'Test Title',
778+
},
779+
},
780+
route: '/organizations/:orgId/explore/metrics/',
781+
},
782+
}
783+
);
784+
785+
const toolbars = screen.getAllByTestId('metric-toolbar');
786+
expect(toolbars).toHaveLength(3);
787+
788+
await waitFor(() => {
789+
expect(
790+
within(toolbars[0]!).getByRole('button', {name: 'metricA'})
791+
).toBeInTheDocument();
792+
});
793+
794+
// Metric A should be disabled because it is referenced by the equation
795+
expect(
796+
within(toolbars[0]!).getByRole('button', {name: 'Delete metric'})
797+
).toBeDisabled();
798+
799+
// Metric B should be enabled because it is not referenced by the equation
800+
expect(
801+
within(toolbars[1]!).getByRole('button', {name: 'Delete metric'})
802+
).toBeEnabled();
803+
804+
// Equation deletion should always be enabled
805+
expect(
806+
within(toolbars[2]!).getByRole('button', {name: 'Delete metric'})
807+
).toBeEnabled();
808+
});
734809
});

0 commit comments

Comments
 (0)