-
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
fix(dashboards): HogQL support for dashboard filters on insights with property group filters #21903
Conversation
) | ||
except Exception as e: | ||
# If pydantic is unhappy about the shape of data, let's ignore property filters and carry on | ||
capture_exception() |
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.
Curious: Do we have Sentry configured to capture exception-level log message as exceptions? If so, we don't need to capture separately, as below is sufficient.
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.
We don't, but there's a way: https://github.com/kiwicom/structlog-sentry
logger.error( | ||
"Failed to apply dashboard property filters", | ||
exception=e, | ||
exc_info=True, |
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.
Can also use logger.exception
and then you don't need the exc_info=True
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.
Oh yeah, that's exactly what I was looking for
Problem
HogQL
apply_dashboard_filters
only worked if the insight's query uses filters in list form – but typically these days, property filter groups are used. This failed (Sentry).Changes
Fixed the bug.
How did you test this code?
Updated
test_insight_refreshing_query
to use a property filter group instead of just list.