feat(insights): hide "other" breakdown #19359
Conversation
mariusandra
changed the title
|
Reminds me of Kibana: 
Maybe we'll also do a toggle? |
|
Size Change: 0 B Total Size: 2 MB ℹ️ View Unchanged
|
| @@ -98,7 +98,6 @@ | |||
| """ | |||
|
|
|||
| TOP_ELEMENTS_ARRAY_OF_KEY_SQL = """ | |||
| SELECT groupArray(value) FROM ( | |||
There was a problem hiding this comment.
Running through groupArray() removes the rows where this field is None. We however do want to know if None is part of the returned fields or not.
| ) | ||
|
|
||
| if filter.using_histogram: | ||
| return response[0][0] | ||
| else: | ||
| return [row[0] for row in response] |
There was a problem hiding this comment.
There are two queries here: 1) integer histogram breakdown, 2) string top results breakdown.
The response of the second query changed in order to also return nulls. More on it below.
| BREAKDOWN_NULL_STRING_LABEL = "$$_posthog_breakdown_null_$$" | ||
| BREAKDOWN_NULL_NUMERIC_LABEL = 9007199254740990 # pow(2, 53) - 2, for JS compatibility |
There was a problem hiding this comment.
I'm not a giant fan of magic strings, but for now I consider this to be good enough... Or at least "helps us today and doesn't block us from adding a proper fix in the future".
|
Finally ready for a review |
|
Tried to test this, but no matter what I do, I don't get the "for readability, not all breakdown values are displayed. Click below to load them." thing locally 🤔 What I still wanted to suggest is, if it wouldn't be better to put the new toggle under options at the top? |
| !breakdown.breakdown_hide_other_aggregation, | ||
| }) | ||
| } | ||
| label='Group renaming values under "Other"' |
There was a problem hiding this comment.
| label='Group renaming values under "Other"' | |
| label='Group remaining values under "Other"' |
|
Thanks for the review! Yes, for now this is only implemented for the non-HogQL trends. I have added "Breakdown controls" as one of the things to fix for the rewrite. I also put an end to the frontend innovation, and moved the button into the sidebar with the other editor filters:
There will likely be some changed snapshots because of this. One of the next things we can add here is the option to toggle exactly how many breakdown values are displayed, and which ones, but that's for future PRs. |
|
Fly-by suggestion: We already have some options relating to breakdowns in the 
|
📸 UI snapshots have been updated15 snapshot changes in total. 0 added, 15 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
|
Something like this? |
📸 UI snapshots have been updated14 snapshot changes in total. 0 added, 14 modified, 0 deleted:
Triggered by this commit. |
Exactly this. |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
| min={1} | ||
| value={breakdownLimit} | ||
| onChange={(newValue) => { | ||
| setBreakdownLimit(newValue ?? 25) |
There was a problem hiding this comment.
What doesn't work here for me is typing a value and then focusing somewhere else, thereby closing the menu. That doesn't seem to trigger onChange.
There was a problem hiding this comment.
Indeed, the UX here is poor. I'll improve it in the next PR as it seems I made the breakdown pills huge now with the new design I didn't have enabled locally
Problem
Not everyone is happy with the "Other" column.
Changes
Adds a way to remove the column when you edit the chart.
I had to make NULL / None handling for breakdowns very explicit as well. Because of our column materialization, we must also treat
""as None.How did you test this code?
Backend tests