-
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: allow hogql property queries in replay filtering #26176
feat: allow hogql property queries in replay filtering #26176
Conversation
@@ -132,7 +132,7 @@ export const universalFiltersLogic = kea<universalFiltersLogicType>([ | |||
newValues.push(newFeatureFlagFilter) | |||
} else { | |||
const propertyType = | |||
item.propertyFilterType ?? taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type) | |||
item?.propertyFilterType ?? taxonomicFilterTypeToPropertyFilterType(taxonomicGroup.type) |
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.
hogql property filters don't have an item so this needs to be slightly safer
@@ -144,6 +147,7 @@ const RecordingsUniversalFilterGroup = (): JSX.Element => { | |||
onRemove={() => removeGroupValue(index)} | |||
onChange={(value) => replaceGroupValue(index, value)} | |||
initiallyOpen={allowInitiallyOpen} | |||
metadataSource={{ kind: NodeKind.RecordingsQuery }} |
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 need something here to correct the autocomplete
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 need a query runner in python for the recordings query
@@ -120,6 +120,8 @@ export function convertUniversalFiltersToRecordingsQuery(universalFilters: Recor | |||
actions.push(f) | |||
} else if (isLogEntryPropertyFilter(f)) { | |||
console_log_filters.push(f) | |||
} else if (isHogQLPropertyFilter(f)) { |
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.
this "just works"™
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.
Does this work for all property types e.g person & event properties? Wondering if the subqueries in the runner affect things here in any way
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.
it works for hogql... am going to ignore everything else for now 🤣
(but noted)
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 would be more worried about it not working for folks without PoE enabled (but maybe that's everyone now)
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the |
Size Change: +110 B (+0.01%) Total Size: 1.11 MB ℹ️ View Unchanged
|
filter = SessionRecordingsFilter(request=request, team=self.team) | ||
self._maybe_report_recording_list_filters_changed(request, team=self.team) | ||
return list_recordings_response(filter, request, self.get_serializer_context()) | ||
use_query_type = (request.GET.get("as_query", "False")).lower() == "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.
let's the API client declare which processing version it accepts so we can test the new mechanism with a slow rollout
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
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.
The test files are a little difficult to review but I'll trust that you've just copied them over and not made any implementational changes.
Left a couple of comments but overall the code / approach looks good. Happy enough that it's behind a FF so I'm sure you'll test it for data accuracy :)
listAPIAsQuery: [ | ||
(s) => [s.featureFlags], | ||
(featureFlags) => { | ||
return !!featureFlags[FEATURE_FLAGS.REPLAY_LIST_RECORDINGS_AS_QUERY] |
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.
One thing that always catches me about the query endpoint is the caching. You might need to disable it so that the list refreshes (or maybe the caching is desirable)
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.
yep, i'm slightly wary of moving to the query endpoint tbh 🙈
(just from a "i don't know how this works right now" viewpoint
@@ -120,6 +120,8 @@ export function convertUniversalFiltersToRecordingsQuery(universalFilters: Recor | |||
actions.push(f) | |||
} else if (isLogEntryPropertyFilter(f)) { | |||
console_log_filters.push(f) | |||
} else if (isHogQLPropertyFilter(f)) { |
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.
Does this work for all property types e.g person & event properties? Wondering if the subqueries in the runner affect things here in any way
operand?: FilterLogicalOperator | ||
session_ids?: string[] | ||
person_uuid?: string | ||
/** | ||
* @default "start_time" | ||
* */ |
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.
TIL
query = RecordingsQuery.model_validate(data_dict) | ||
query.session_ids = playlist_items |
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.
Are we only doing this validation because it doesn't go through the /query
endpoint? Maybe we should just move it to a runner instead. Totally happy if that's the next step in a followup PR
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.
am going to create a runner so i get autocomplete for the hogql property box
but want to understand that way more before I move this API over 😨
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
we don't allow people to add hogql property filters in replay but we could
this PR
## for this PR
to do in follow-up since I want to test the new stuff in prod and everything is behind flags
QueryRunner
forRecordingsQuery
hogql filtering