-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
feat(insights): Switch to ratios for slow and frozen frames #74523
Conversation
@@ -819,6 +819,10 @@ export function getSortField( | |||
return field; | |||
} | |||
|
|||
if (field.startsWith('division_if(')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like a hack 🙈, an alternative would be adding them to aggregations, but then we'd need to support this for discover queries as well (see count_if as an example,
sentry/static/app/utils/discover/fields.tsx
Lines 291 to 318 in d10148c
[AggregationKey.COUNT_IF]: { | |
...getDocsAndOutputType(AggregationKey.COUNT_IF), | |
parameters: [ | |
{ | |
kind: 'column', | |
columnTypes: validateDenyListColumns( | |
['string', 'duration', 'number'], | |
['id', 'issue', 'user.display'] | |
), | |
defaultValue: 'transaction.duration', | |
required: true, | |
}, | |
{ | |
kind: 'dropdown', | |
options: CONDITIONS_ARGUMENTS, | |
dataType: 'string', | |
defaultValue: CONDITIONS_ARGUMENTS[0].value, | |
required: true, | |
}, | |
{ | |
kind: 'value', | |
dataType: 'string', | |
defaultValue: '300', | |
required: true, | |
}, | |
], | |
isSortable: true, | |
multiPlotType: 'area', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a cleaner way to get around this, but I have to do a bit of investigation. I'm seeing some odd access paths when it comes to the structure of tableMeta
. If you want, we can remove this until I get a chance to poke around a bit more and we can put up a separate PR to handle the sortability of the headers.
The endpoint returns in the meta data that the corresponding type to the division_if
statements should be percentages so they should be sortable by default when it gets to this block. I know that the key gets kind of mangled for equations but we should be able to work around that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few comments. I think the conversion for the table looks good but there's probably an alternate way to access the tooltips without editing the type and also fixing the sorting without hardcoding the division_if
prefix.
I think we should pull out the field renderer part and have a separate PR. I can take a look deeper tomorrow on how to get around it
static/app/views/insights/mobile/common/components/tables/screensTable.tsx
Outdated
Show resolved
Hide resolved
@@ -819,6 +819,10 @@ export function getSortField( | |||
return field; | |||
} | |||
|
|||
if (field.startsWith('division_if(')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's a cleaner way to get around this, but I have to do a bit of investigation. I'm seeing some odd access paths when it comes to the structure of tableMeta
. If you want, we can remove this until I get a chance to poke around a bit more and we can put up a separate PR to handle the sortability of the headers.
The endpoint returns in the meta data that the corresponding type to the division_if
statements should be percentages so they should be sortable by default when it gets to this block. I know that the key gets kind of mangled for equations but we should be able to work around that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! thanks for splitting out the sorting changes we discussed
As described here, we want to move to ratios for slow and frozen frames.
Utilizes division_if to calculate the ratios.