-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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(hogql): run filter based insights via hogql #17611
Conversation
40709da
to
258b88a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't run, but looks good overall.
Will this force us to enable/disable hogql insights per team? We might want to still run them per user, as they're even too raw for usage within the company?
👍
I think so, yes. Will look into adding a user based version as well. |
Turns out we can only base the feature flags on users OR teams, not both. Thus changed the logic to rely on users for now. We can |
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
This reverts commit cb22ba7.
Problem
We're still using the legacy insight query when fetching results from the insights api.
Changes
This PR
HOGQL_INSIGHTS_OVERRIDE
with implementation similar to the PoE oneinsight_result
serializer method to use an alternative path for insights that support it, when the flag is onHow did you test this code?
Added a test and verified manually for the lifecycle query. There are still some rough edges e.g. the demo test account filter doesn't work as the validation breaks for cohort filters.
Don't forget to set
"HOGQL_INSIGHTS_OVERRIDE": "True"