Skip to content

feat(tracemetrics): Do not allow deletion of metrics used in equations#112893

Open
narsaynorath wants to merge 4 commits intomasterfrom
nar/feat/tracemetrics-do-not-allow-deletion-of-metrics-in-equations
Open

feat(tracemetrics): Do not allow deletion of metrics used in equations#112893
narsaynorath wants to merge 4 commits intomasterfrom
nar/feat/tracemetrics-do-not-allow-deletion-of-metrics-in-equations

Conversation

@narsaynorath
Copy link
Copy Markdown
Member

To avoid invalid states, avoid deleting metrics that are used in equations.

@narsaynorath narsaynorath requested a review from a team as a code owner April 14, 2026 03:24
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Apr 14, 2026
@sentry
Copy link
Copy Markdown
Contributor

sentry bot commented Apr 14, 2026

Sentry Snapshot Testing

Name Added Removed Modified Renamed Unchanged Status
sentry-frontend
sentry-frontend
0 0 0 0 204 ✅ Unchanged

return (
v &&
isVisualizeEquation(v) &&
v.expression.tokens.some(token => token.text === functionString)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[Bug] You mentioned that this has a bug if two metrics have the same stringification text. ACK that I can reproduce it: https://sentry-781r3afew.sentry.dev/explore/metrics/?metric=%7B%22metric%22%3A%7B%22name%22%3A%22dashboards.seer.generation.session.error%22%2C%22type%22%3A%22counter%22%2C%22unit%22%3A%22none%22%7D%2C%22query%22%3A%22%22%2C%22aggregateFields%22%3A%5B%7B%22yAxes%22%3A%5B%22sum%28value%2Cdashboards.seer.generation.session.error%2Ccounter%2Cnone%29%22%5D%7D%5D%2C%22aggregateSortBys%22%3A%5B%7B%22field%22%3A%22sum%28value%2Cdashboards.seer.generation.session.error%2Ccounter%2Cnone%29%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22sortBys%22%3A%5B%7B%22field%22%3A%22timestamp%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22mode%22%3A%22samples%22%7D&metric=%7B%22metric%22%3A%7B%22name%22%3A%22dashboards.seer.generation.session.error%22%2C%22type%22%3A%22counter%22%2C%22unit%22%3A%22none%22%7D%2C%22query%22%3A%22%22%2C%22aggregateFields%22%3A%5B%7B%22yAxes%22%3A%5B%22sum%28value%2Cdashboards.seer.generation.session.error%2Ccounter%2Cnone%29%22%5D%7D%5D%2C%22aggregateSortBys%22%3A%5B%7B%22field%22%3A%22sum%28value%2Cdashboards.seer.generation.session.error%2Ccounter%2Cnone%29%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22sortBys%22%3A%5B%7B%22field%22%3A%22timestamp%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22mode%22%3A%22samples%22%7D&metric=%7B%22metric%22%3A%7B%22name%22%3A%22%22%2C%22type%22%3A%22%22%7D%2C%22query%22%3A%22%22%2C%22aggregateFields%22%3A%5B%7B%22yAxes%22%3A%5B%22equation%7C%22%5D%7D%5D%2C%22aggregateSortBys%22%3A%5B%7B%22field%22%3A%22equation%7C%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22sortBys%22%3A%5B%7B%22field%22%3A%22timestamp%22%2C%22kind%22%3A%22desc%22%7D%5D%2C%22mode%22%3A%22samples%22%7D

Using some sort of entity-specific ID (not order-dependent labels) would be good IMO.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@JoshuaKGoldberg Okay I think I fixed this! Before I was just naively doing a string match on the aggregate in string form, but this meant that queries that were the same would just be treated as "in-use". Now the equation builder calls a callback to set what references are being used and then passes down the set of used references that I can use with the query label to know which one is used. Would you mind helping me with a review when you get a chance? Thanks!

@narsaynorath
Copy link
Copy Markdown
Member Author

Drafting to fix merge conflicts, fix bug, etc

@narsaynorath narsaynorath marked this pull request as draft April 14, 2026 16:04
Base automatically changed from nar/feat/tracemetrics-use-references-as-intermediate-representation to master April 14, 2026 16:48
@narsaynorath narsaynorath force-pushed the nar/feat/tracemetrics-do-not-allow-deletion-of-metrics-in-equations branch from 372190a to 9f13618 Compare April 14, 2026 17:47
@narsaynorath narsaynorath marked this pull request as ready for review April 14, 2026 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Frontend Automatically applied to PRs that change frontend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants