Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ref(starfish): Show 3 significant digits for queries per minute #54033

Merged
merged 4 commits into from
Aug 3, 2023
Merged

ref(starfish): Show 3 significant digits for queries per minute #54033

merged 4 commits into from
Aug 3, 2023

Conversation

AbhiPrasad
Copy link
Member

@AbhiPrasad AbhiPrasad commented Aug 2, 2023 •
edited
Loading

  1. Add precision arg to formatAbbreviatedNumber to set # of significant digits
  2. Update getFieldRenderer renderer baggage to use precision, which is what starfish spans table consumes Update getFieldRenderer rate renderer to use constant precision of 3 for sig digs.

Before

image

After

image

@AbhiPrasad AbhiPrasad requested review from a team August 2, 2023 16:49
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Aug 2, 2023
gggritso
gggritso reviewed Aug 2, 2023
View reviewed changes
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

LGTM, but one question! Do you think these changes to baggage are worth it? It's a bit confusing, and all rates should always have the same number of significant digits anyway, so it actually might be good if we hard-code to 3 rather than accepting a parameter there! Thoughts?

@AbhiPrasad
Copy link
Member Author

Do you think these changes to baggage are worth it

I did this because getFieldRenderer has more consumers than just starfish, and I didn't want to cause any regressions/confusion for any of the other views that depend on it.

I do agree hardcoding is probably the better approach though, so if you think the impact on the other views is fine, I can amend the PR!

@gggritso
Copy link
Member

gggritso commented Aug 2, 2023

@AbhiPrasad as long as the change only affects the rate renderer, it should be good to amend! The only affected fields in the tables are tpm(), sps() and so on, and they'd all benefit from this precision change

@AbhiPrasad
Copy link
Member Author

Updated!

gggritso
gggritso reviewed Aug 3, 2023
View reviewed changes
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

Sorry to keep annoying you with this, but I think there's an even better way because it'll both enforce the right number of significant digits and use the right locale-based number formatting

static/app/utils/formatters.spec.tsx Show resolved Hide resolved
static/app/utils/formatters.tsx Outdated Show resolved Hide resolved
gggritso
gggritso approved these changes Aug 3, 2023
View reviewed changes
Copy link
Member

@gggritso gggritso left a comment

Choose a reason for hiding this comment

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

🚢

@AbhiPrasad AbhiPrasad merged commit 16c86c1 into master Aug 3, 2023
49 checks passed
@AbhiPrasad AbhiPrasad deleted the abhi-qpm-format-sig-dig branch August 3, 2023 12:59
@AbhiPrasad AbhiPrasad added the Trigger: Revert add to a merged PR to revert it (skips CI) label Aug 3, 2023
@getsentry-bot
Copy link
Contributor

PR reverted: dd75a2c

getsentry-bot added a commit that referenced this pull request Aug 3, 2023
@getsentry-bot @AbhiPrasad
Revert "ref(starfish): Show 3 significant digits for queries per minu…
dd75a2c 
…te (#54033)"

This reverts commit 16c86c1.

Co-authored-by: AbhiPrasad <[email protected]>
@AbhiPrasad AbhiPrasad mentioned this pull request Aug 3, 2023
Merged
AbhiPrasad added a commit that referenced this pull request Aug 3, 2023
@AbhiPrasad
ref(starfish): Show 3 significant digits for queries per minute try 2 (…
9696656 
…#54100)

This was previously done and merged in with
#54033, but had to be reverted
with
dd75a2c
because of
[JAVASCRIPT-2NGX](https://sentry.sentry.io/issues/4367379265/).

The fix for the Sentry issue was related to
`static/app/views/performance/transactionSummary/transactionVitals/vitalCard.tsx`,
which was blindly forwarding the echarts formatter args to
`formatAbbreviatedNumber`. When we introduced a new precision arg to
`formatAbbreviatedNumber`, the echarts arg was being used, which would
lead to unexpected values being provided to the function.

fixes JAVASCRIPT-2NGX
fixes JAVASCRIPT-2NGW
@github-actions github-actions bot locked and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gggritso gggritso gggritso approved these changes

Assignees
No one assigned
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AbhiPrasad @gggritso @getsentry-bot