Add a filter function to plugins to filter out events so that they are not tracked - #1326
Conversation
7b880c9 to
530094f
Compare
BundleMonFiles added (6)
Total files change +101.16KB 0% Final result: ❌ View report in BundleMon website ➡️ |
| // Call the filter on plugins to determine if the event should be tracked | ||
| const skip = corePlugins.find((plugin) => { | ||
| try { | ||
| return plugin.filter && !plugin.filter(pb.build()); |
There was a problem hiding this comment.
Should we also pass the SDJs explicitly? I always find working with Payload directly kind of awkward since you have to check for ue_px/cx and possibly decode them (and since URL safe base64 isn't usually covered by btoa you may require a dependency for that)
There was a problem hiding this comment.
Would it be better to explicitly check for === false here to avoid accidents? Or is skipping on falsey values like undefined/null a useful convenience?
There was a problem hiding this comment.
Should we also pass the SDJs explicitly? I always find working with Payload directly kind of awkward since you have to check for ue_px/cx and possibly decode them (and since URL safe base64 isn't usually covered by btoa you may require a dependency for that)
This is a very good point, I agree that it's super annoying to work with the Payload and the PayloadBuilder. I think this is something we can improve with some breaking changes in the v4 tracker. I want to add a getContextEntities and getSelfDescribing event function to the PayloadBuilder and pass that information along with the payload to afterTrack and filter here but in order to do that we need to make changes to the PayloadBuilder (so that we don't have to parse the cx and ue_px values). I will raise a separate PR to do this.
Would it be better to explicitly check for === false here to avoid accidents? Or is skipping on falsey values like undefined/null a useful convenience?
I kind of think that skipping falsey values is useful as we have been getting these tickets where people use JS or ignore the types and just return null where we don't expect it...
There was a problem hiding this comment.
I kind of think that skipping falsey values is useful as we have been getting these tickets where people use JS or ignore the types and just return null where we don't expect it...
Haha, that's exactly why I think we should be more pedantic about it. 😂
"Oh we goofed our filter function and accidentally tracked some events we didn't mean to" vs "FFFFFUUUUU we goofed our filter function and have irrevocably lost a bunch of events that were important!".
Though I guess once someone inevitably uses this for PII removal the situation will be reversed so we can't really win.
I was thinking that rather than split filter out we just make it so beforeTrack can throw Error("SKIP_EVENT") or something very specific and if we get that exact exception we do the skip. More explicit, harder to do anything accidentally.
PayloadBuilder changes sound great!
There was a problem hiding this comment.
You are right, I didn't think this through, checking for === false is safer in this case 😅 Will change that.
About the idea to throw an error from the beforeTrack function instead of having a dedicated filter one – that could work too, but I prefer filter for two reasons:
- We can ensure that the
filteris called after all pluginbeforeTrackfunctions are applied and thus the payload is kind of complete. So one can check for more things, like context entities added by other plugins in the filter function. - We have the same
filterAPI in the plugins on mobile trackers so it's nice to be consistent on the API. It also makes it a bit more obvious that it is possible to filter events.
2 participants
Adds a new
filter(payload)function to plugins which accepts the payload and returns a boolean deciding whether the event should be tracked or skipped. This has various use cases such as:In addition to the new function, I had to change the return type of the
track()function in the tracker core. Previously it always returnedPayload. Now in case an event is filtered out, the function returnsundefined. This should not affect the usage of the browser or node trackers, but still is a breaking change so adding this to v4.