Skip to content

fix(tracemetrics): Drop placeholder unit and always use none#116007

Merged
narsaynorath merged 7 commits into
masterfrom
nar/fix/tracemetrics-drop-placeholder-type-and-treat-none
May 21, 2026
Merged

fix(tracemetrics): Drop placeholder unit and always use none#116007
narsaynorath merged 7 commits into
masterfrom
nar/fix/tracemetrics-drop-placeholder-type-and-treat-none

Conversation

@narsaynorath
Copy link
Copy Markdown
Member

@narsaynorath narsaynorath commented May 21, 2026

Since we don't want to use - anymore as a placeholder, we should really be coercing any null values to the string "none", which adds special filters in both the frontend and backend code.

This PR removes all fallbacks to - so we can just continue passing "none" along in the future. Anything that's saved also gets coerced to "none" when editing, which is the behaviour we want moving forward.

I also de-duplicate selector options so the edge case when a metric name and type return with null and "none" units simultaneously get treated as the same value.

All of the tests are also updated to drop using - and use "none". There's a test added that shows stuff that still uses - gets changed into "none"

@narsaynorath narsaynorath requested review from a team as code owners May 21, 2026 14:45
@github-actions github-actions Bot added the Scope: Frontend Automatically applied to PRs that change frontend components label May 21, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 21, 2026

📊 Type Coverage Diff

✅ No new type safety issues introduced. Coverage: 93.60%

Comment thread static/app/views/explore/metrics/metricToolbar/metricSelector/metricSelector.tsx Outdated
@narsaynorath narsaynorath marked this pull request as draft May 21, 2026 14:50
@narsaynorath narsaynorath changed the title fix(tracemetrics): Drop placeholder type and always use none fix(tracemetrics): Drop placeholder unit and always use none May 21, 2026
@narsaynorath narsaynorath marked this pull request as ready for review May 21, 2026 15:23
@narsaynorath narsaynorath requested a review from a team as a code owner May 21, 2026 15:23
Copy link
Copy Markdown
Contributor

@nsdeschenes nsdeschenes left a comment

Choose a reason for hiding this comment

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

Worth taking a peek at those cursor comments 👀

But from my review, things are looking good 👍

@narsaynorath
Copy link
Copy Markdown
Member Author

narsaynorath commented May 21, 2026

@cursor review (previous comment might be stale)

Comment thread static/app/views/dashboards/widgetBuilder/utils/buildTraceMetricAggregate.ts Outdated
Copy link
Copy Markdown
Contributor

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 77c2c86. Configure here.

Comment thread static/app/views/explore/metrics/utils.tsx Outdated
@narsaynorath narsaynorath merged commit 22fbb56 into master May 21, 2026
71 checks passed
@narsaynorath narsaynorath deleted the nar/fix/tracemetrics-drop-placeholder-type-and-treat-none branch May 21, 2026 16:51
JonasBa pushed a commit that referenced this pull request May 21, 2026
Since we don't want to use `-` anymore as a placeholder, we should
really be coercing any `null` values to the string `"none"`, which adds
special filters in both the frontend and backend code.

This PR removes all fallbacks to `-` so we can just continue passing
`"none"` along in the future. Anything that's saved also gets coerced to
`"none"` when editing, which is the behaviour we want moving forward.

I also de-duplicate selector options so the edge case when a metric name
and type return with `null` and `"none"` units simultaneously get
treated as the same value.

All of the tests are also updated to drop using `-` and use `"none"`.
There's a test added that shows stuff that still uses `-` gets changed
into `"none"`
natemoo-re pushed a commit that referenced this pull request May 21, 2026
Since we don't want to use `-` anymore as a placeholder, we should
really be coercing any `null` values to the string `"none"`, which adds
special filters in both the frontend and backend code.

This PR removes all fallbacks to `-` so we can just continue passing
`"none"` along in the future. Anything that's saved also gets coerced to
`"none"` when editing, which is the behaviour we want moving forward.

I also de-duplicate selector options so the edge case when a metric name
and type return with `null` and `"none"` units simultaneously get
treated as the same value.

All of the tests are also updated to drop using `-` and use `"none"`.
There's a test added that shows stuff that still uses `-` gets changed
into `"none"`
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