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(web-analytics): Support diff on Web Goals dashboard #26999

Merged
merged 2 commits into from
Dec 18, 2024

Conversation

rafaeelaudibert
Copy link
Member

Problem

We were not computing diff for the web goals section of our dashboard. This wasn't detected earlier because we don't have any actions on our local setup - we should change that.

Changes

Besides adding diff, we're also now properly formatting it in the UI, it was previously using the old formatting, therefore percentages were not accurate.

Before After
image image

After doesn't have data, but you can see it's properly formatted

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

Yep

How did you test this code?

Manual UI test + updated automated tests

We were not computing diff for the web goals section of our dashboard. This wasn't detected earlier because we don't have any actions on our local setup - we should change that.

Besides adding diff, we're also now properly formatting it in the UI, it was previously using the old formatting, therefore percentages were not accurate.
@rafaeelaudibert rafaeelaudibert enabled auto-merge (squash) December 18, 2024 03:39
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 haven't run this locally, but LGTM

@@ -25,93 +34,153 @@ class WebGoalsQueryRunner(WebAnalyticsQueryRunner):
cached_response: CachedWebGoalsQueryResponse

def to_query(self) -> ast.SelectQuery | ast.SelectSetQuery:
with self.timings.measure("actions"):
Copy link
Member

Choose a reason for hiding this comment

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

In case you're wondering why we time so much:

We used to use a python version of ANTLR to parse HogQL, and it was really slow, to the point where it would often take just as long as the actual query on clickhouse!

Since then, we've switched to the C++ version, which is way faster, but a lot of these timing statements come from the times before that (or from copying code from that era).

Hopefully, loading the actions from Postgres should be fast!

Copy link
Member Author

Choose a reason for hiding this comment

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

Aah, I thought they were just useful in general, so that's why I added that hahah

I will keep it in mind :)

@rafaeelaudibert rafaeelaudibert merged commit 2a80480 into master Dec 18, 2024
91 of 92 checks passed
@rafaeelaudibert rafaeelaudibert deleted the fix-web-goals-percentage-computation branch December 18, 2024 13:36
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.

2 participants