test(insights): Run all tests with filters-based insights using HogQL
#21861
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
Twixes
force-pushed
the
hogql-insights-ci
branch
2 times, most recently
from
1cac259 to
dd58b80
Compare
Twixes
force-pushed
the
query-in-background-reloads
branch
2 times, most recently
from
9ce7475 to
f09821a
Compare
Twixes
force-pushed
the
hogql-insights-ci
branch
2 times, most recently
from
1d83564 to
50a471f
Compare
Twixes
commented
May 23, 2024
| @@ -326,7 +326,6 @@ def create(self, validated_data: dict, *args: Any, **kwargs: Any) -> Insight: | |||
| raise serializers.ValidationError("Dashboard not found") | |||
|
|
|||
| DashboardTile.objects.create(insight=insight, dashboard=dashboard, last_refresh=now()) | |||
| insight.last_refresh = now() # set last refresh if the insight is on at least one dashboard | |||
There was a problem hiding this comment.
This has not been used in a long time and not sure it ever actually worked. The actual last_refresh is in InsightCachingState at the insight level, and in the cached result packages themselves
Twixes
commented
May 23, 2024
Comment on lines
+151
to
+157
| if isinstance(cache_key, str) and isinstance(last_refresh, str): | ||
| update_cached_state( # Updating the relevant InsightCachingState | ||
| insight.team_id, | ||
| cache_key, | ||
| last_refresh, | ||
| result=None, # Not caching the result here, since in HogQL this is the query runner's responsibility | ||
| ) |
There was a problem hiding this comment.
The only behavioral change in this PR: TestDashboard::test_refresh_cache revealed we weren't updating InsightCachingState in here. This change brings us fully back on track with tracking which insights need a background refresh
Twixes
changed the title
filters-based insights using HogQL
thmsobrmlr
approved these changes
May 23, 2024
There was a problem hiding this comment.
Changes look good. 👍 to merge this in once tests pass
This will allow us to catch problems in the saved insights/dashboards layer, and later on when converting /insight/{trend,funnel,etc}.
I don't think we ever want to convert these, since they are replaced by /query, right?
|
As for
What do you think @thmsobrmlr? Let me know if you have an idea to refine this |
xrdt
pushed a commit
that referenced
this pull request
May 24, 2024
…QL (#21861) * ci: Run core backend tests with both HogQL and legacy insights * Double the number of tests * Fix env var setting * Add some HOGQL_INSIGHTS_OVERRIDE overrides in tests * Mark `Insight.last_refresh` as deprecated * Fix bad merge * Update query snapshots * Update query snapshots * Update test_team.py * Actually remove legacy backend from matrix * Update test_fetch_from_cache.py * Use `update_cached_state` in `calculate_for_query_based_insight()` * Clean up CI changes and a comment * Fix `update_cached_state` typing * Update test_fetch_from_cache.py * Update test_insight_cache.py * Update test_insight_cache.py * Clarify `generate_insight_cache_key` as legacy function --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
timgl
pushed a commit
that referenced
this pull request
May 24, 2024
…QL (#21861) * ci: Run core backend tests with both HogQL and legacy insights * Double the number of tests * Fix env var setting * Add some HOGQL_INSIGHTS_OVERRIDE overrides in tests * Mark `Insight.last_refresh` as deprecated * Fix bad merge * Update query snapshots * Update query snapshots * Update test_team.py * Actually remove legacy backend from matrix * Update test_fetch_from_cache.py * Use `update_cached_state` in `calculate_for_query_based_insight()` * Clean up CI changes and a comment * Fix `update_cached_state` typing * Update test_fetch_from_cache.py * Update test_insight_cache.py * Update test_insight_cache.py * Clarify `generate_insight_cache_key` as legacy function --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Let's make the HogQL engine the default. Still leaving a strategy with the legacy engine running.
This will allow us to catch problems in the saved insights/dashboards layer, and later on when converting
/insight/{trend,funnel,etc}.