-
Notifications
You must be signed in to change notification settings - Fork 5
Conversation
All matching options are functional (selector matching was definitely the trickiest). Any optimization tips appreciated. |
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 is amazing work!
Initial first pass - didn't compare anything with the Python code yet, and didn't look over everything just yet.
This is great stuff though.
requirements: Partial<Element> | ||
|
||
constructor(tag: string, directDescendant: boolean, escapeSlashes: boolean) { | ||
const SELECTOR_ATTRIBUTE_REGEX = /([a-zA-Z]*)\[(.*)=[\'|\"](.*)[\'|\"]\]/ |
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.
A comment here would be useful
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.
What should this comment convey?
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 think with longer regexes an example of what it should match is always useful
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 is some really nice code. Well done! :)
I found one TODO to fix, and have some thoughts regarding caching person. Otherwise LGTM.
case 'event': | ||
return this.checkEventAgainstEventFilter(event, filter) | ||
case 'person': | ||
person = await this.db.fetchPerson(event.team_id, event.distinct_id) | ||
return this.checkEventAgainstPersonFilter(person, filter) | ||
case 'element': | ||
return this.checkEventAgainstElementFilter(elements, filter) | ||
case 'cohort': | ||
person = await this.db.fetchPerson(event.team_id, event.distinct_id) | ||
return await this.checkEventAgainstCohortFilter(person, filter) |
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 db.fetchPerson
could be fetching the same db rows multiple times per ActionMatcher.match
in case we have a lot of steps that check user properties.
It would be great if we could either recycle the person that we got while ingesting, or then just cache it here somehow.
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.
True. I was initially thinking of just using the person from processEvent
, but keeping track of that object while making sure it's updated along with property updates etc. all while other events are being processed would very likely result in some inconsistencies. So the person for action matching is fetched once per event now. There could be some better optimizations too, but how about seeing how this performs dryrunning in production?
* Reorient `ActionManager` to group by teamId for practicality * Make `getTeamActions()` return type more versatile * Add ActionMatcher base * Add `matchActions` worker task for optimization * Add part of action matching checks * Fix PubSub's lack of teamId * Remove `eventsProcessor.prepare()` calls * Add a legit ActionMatcher test * Adjust test for matchActions task * Improve task counting in test * Add moar matching capabilities * Improve tests * Reorganize class dependencies * Add cohort matching * Add element/selector matching and polish other action matching parts * Handle selector matching edge cases * Save matched action occurrences to Postgres * Fix action-matcher tests * Don't expose ActionManager methods in ActionMatcher * Use feedback + RE2 * Remove never satisfied branch * Address sum feedback and clean code up * Use action matching results in a smarter way * Fix `createHub` * Add action matching metric * Enhance `PLUGIN_SERVER_ACTION_MATCHING` * Update action-matcher.test.ts * Remove `||=` * Update process-event.ts * Only fetch person if matching actions * Fix non-string distinct ID handling * Update process-event.ts
Changes
Resolves #235.
In-memory synced list of action steps for all teams, reloading/adding/removing data for a specific action based on pubsub dispatched when the action is updated in the app, using Django model signalsIn-memory action definitions synced with Django #403 & ReorientActionManager
to group byteamId
for practicality #433$pageview
)posthog_action_events
table (Postgres)Checklist