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: error clustering UI #20958

Merged
merged 18 commits into from
Mar 19, 2024
Merged

feat: error clustering UI #20958

merged 18 commits into from
Mar 19, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Mar 15, 2024

Problem

The first UI needed some work

Changes

Screenshot 2024-03-15 at 19 07 31
  • Use a table to allow ordering
  • Add JSON parsing to error (mainly for stack traces)
  • Add % count of sessions viewed with the error

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

👍

How did you test this code?

]
def construct_response(df, team, user):
viewed_session_ids = (
SessionRecordingViewed.objects.filter(team=team, user=user, session_id__in=df["session_id"].unique())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this list be potentially too long to be passed to Postgres?

Copy link
Member

Choose a reason for hiding this comment

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

I think because we're scoping it to team and individual user we should be ok...

Could write a test to see how many session ids before we get an error but it's still not generally released so we should be ok

Copy link
Contributor

github-actions bot commented Mar 15, 2024

Size Change: 0 B

Total Size: 821 kB

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

compressed-size-action

@daibhin daibhin requested a review from a team March 18, 2024 15:38
@marandaneto
Copy link
Member

marandaneto commented Mar 18, 2024

For the row's description when it's an error, since we know it's a stack trace and the first line is the "human readable" version of the error, instead of setting the whole stringified JSON, can we set just the first line?

eg:
{error: failed to fetch\n at bla}
to
failed to fetch

@daibhin
Copy link
Contributor Author

daibhin commented Mar 18, 2024

@marandaneto great suggestion! I added the title parsing here: 2142c82. Only checking for the error key right now which admittedly might be very hit or miss (specific to our product e.g. things like kea, logger setup, etc) but we can add more keys over time if customers find it useful

Copy link
Member

@pauldambra pauldambra left a comment

Choose a reason for hiding this comment

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

one text suggestion but otherwise 🚢

]
def construct_response(df, team, user):
viewed_session_ids = (
SessionRecordingViewed.objects.filter(team=team, user=user, session_id__in=df["session_id"].unique())
Copy link
Member

Choose a reason for hiding this comment

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

I think because we're scoping it to team and individual user we should be ok...

Could write a test to see how many session ids before we get an error but it's still not generally released so we should be ok

function parseTitle(error: string): string {
try {
const parsedError = JSON.parse(error)
return parsedError.error || error
Copy link
Member

Choose a reason for hiding this comment

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

I'd still just get the 1st line here since the error is always a stack trace.

@marandaneto
Copy link
Member

@marandaneto great suggestion! I added the title parsing here: 2142c82. Only checking for the error key right now which admittedly might be very hit or miss (specific to our product e.g. things like kea, logger setup, etc) but we can add more keys over time if customers find it useful

ah gotcha, I thought that was the general structure for all errors.

@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.

@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.

@pauldambra
Copy link
Member

should we get this in?

@daibhin
Copy link
Contributor Author

daibhin commented Mar 19, 2024

@pauldambra yep, E2E Cypress tests have failed a couple of times now. Rerunning them but might have to do so locally if they don't work this time

@pauldambra
Copy link
Member

Ah, they've passed now but it was the accidentally required depot steps failing, sorry!

@daibhin daibhin merged commit 088399b into master Mar 19, 2024
130 checks passed
@daibhin daibhin deleted the dn-feat/error-clustering-ui branch March 19, 2024 16:02
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.

4 participants