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.

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): string breakdowns #21023

Merged
merged 35 commits into from
Mar 21, 2024
Merged

feat(insights): string breakdowns #21023

merged 35 commits into from
Mar 21, 2024

Conversation

mariusandra
Copy link
Collaborator

@mariusandra mariusandra commented Mar 19, 2024

Problem

Breakdowns with "other", when the value is returned in a funny format, such as a list ['array of text'], cause type errors when passing through the transform(value, [['txt']], [['txt']], 'other') function that adds the "other"

The only reason I initially added BREAKDOWN_NULL_NUMERIC_LABEL vs BREAKDOWN_NULL_STRING_LABEL was because not everything was returned as a string, and I wanted to preserve sorting when things were returned as numbers, and manually sorted. However other than strings, ints and floats, there are many other types of breakdown values that can be returned... arrays, tuples, bools, datetimes, etc.

Changes

  • Change all breakdown values to be returned as strings. The only exception is histogram breakdowns.
  • Add "natural sorting" to the insight details table. This way we can still sort breakdowns returned as numbers or arrays in an order that makes sense.
  • Seems like breakdown values weren't unbounded to 25 when using HogQL breakdowns. Now they are.
  • Fixes a whole bunch of tickets from the HogQL trends errors table
  • Changes only flagged code.

Does this work well for both Cloud and self-hosted?

yes

How did you test this code?

  • Plenty of tests got updated in the process
  • Clicked into the actors modal and everything still worked

@mariusandra mariusandra changed the title Only strings feat(insights): string breakdowns Mar 19, 2024
@mariusandra mariusandra marked this pull request as ready for review March 21, 2024 15:30
Copy link
Contributor

github-actions bot commented Mar 21, 2024

Size Change: 0 B

Total Size: 824 kB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 824 kB

compressed-size-action

@mariusandra mariusandra changed the base branch from master to insight-moar-fixas March 21, 2024 15:33
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Base automatically changed from insight-moar-fixas to master March 21, 2024 17:09
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

1 snapshot changes in total. 0 added, 1 modified, 0 deleted:

  • chromium: 0 added, 1 modified, 0 deleted (diff for shard 2)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@mariusandra mariusandra requested review from robbie-c and a team March 21, 2024 22:00
Copy link
Member

@robbie-c robbie-c left a comment

Choose a reason for hiding this comment

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

I don't know this code super well but LGTM after ~10 min review, the described change makes sense

@mariusandra
Copy link
Collaborator Author

Thanks 🙌.

Happy to update/revert if anyone has additional thoughts. I want to see if the changes here make a difference, so going to merge it in.

@mariusandra mariusandra merged commit fa5a1d6 into master Mar 21, 2024
134 checks passed
@mariusandra mariusandra deleted the only-strings branch March 21, 2024 22:55
Copy link

sentry-io bot commented Mar 22, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ CHQueryErrorTimeoutExceeded: Code: 159. /api/projects/{parent_lookup_team_id}/query/ View Issue
  • ‼️ CHQueryErrorTimeoutExceeded: Code: 159. /api/projects/{parent_lookup_team_id}/query/ View Issue
  • ‼️ CHQueryErrorIllegalColumn: Code: 44. /api/projects/{parent_lookup_team_id}/query/ View Issue
  • ‼️ CHQueryErrorNumberOfArgumentsDoesntMatch: Code: 42. /api/projects/{parent_lookup_team_id}/query/ View Issue
  • ‼️ CHQueryErrorUnknownIdentifier: Code: 47. /api/projects/{parent_lookup_team_id}/query/ View Issue

Did you find this useful? React with a 👍 or 👎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants