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(errors): Allow searching for errors #25175

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Conversation

neilkakkar
Copy link
Collaborator

@neilkakkar neilkakkar commented Sep 24, 2024

Problem

image

^^ Opted to add this to the query runner rather than manipulate filter groups in the frontend because I imagine this can get complicated. Easier to isolate, test and update this logic as and when the search function changes in the backend.

Changes

👉 Stay up-to-date with PostHog coding conventions for a smoother review.

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

How did you test this code?

Copy link
Contributor

github-actions bot commented Sep 24, 2024

Size Change: 0 B

Total Size: 9.21 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 9.21 MB

compressed-size-action

@neilkakkar neilkakkar marked this pull request as ready for review September 26, 2024 09:27
@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

Copy link
Contributor

@daibhin daibhin left a comment

Choose a reason for hiding this comment

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

Some minor things but overall looks good. Approving for when you're happy with the debouncing, nothing else blocking

@@ -1523,6 +1523,7 @@ export interface ErrorTrackingQuery extends DataNode<ErrorTrackingQueryResponse>
assignee?: integer | null
filterGroup?: PropertyGroupFilter
filterTestAccounts?: boolean
searchQuery?: string
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed this makes sense as being separate to the filterGroup

posthog/hogql_queries/error_tracking_query_runner.py Outdated Show resolved Hide resolved
Comment on lines +135 to +136
ast.CompareOperation(
op=ast.CompareOperationOp.Gt,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a concern for right now but maybe we could consider some degree of fuzzy matching in future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nice 👌


self.assertEqual(len(results), 1)
self.assertEqual(results[0]["fingerprint"], ["DatabaseNotFound"])
self.assertEqual(results[0]["occurrences"], 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the negative case is only implicitly tested e.g. what happens when the search query doesn't match - do we get 0 results?

@@ -37,7 +37,9 @@ def to_query(self) -> ast.SelectQuery:

def select(self):
exprs: list[ast.Expr] = [
ast.Alias(alias="occurrences", expr=ast.Call(name="count", args=[])),
ast.Alias(
alias="occurrences", expr=ast.Call(name="count", distinct=True, args=[ast.Field(chain=["uuid"])])
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@daibhin found that due to the left outer join for overrides, sometimes events are double counted, which makes this new test I added flakey. Not sure why this doesn't happen on existing tests 🤔 , maybe to do with how/when overrides are set, since I add events after the first flush.

But anyhow, should make this case clear where we never doublecount events.

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

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

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

Triggered by this commit.

👉 Review this PR's diff of snapshots.

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