Support destination filters for device mode action destinations #597

danieljackins · 2022-09-15T16:42:58Z

This PR allows destination filters to work with action destinations, by adding destination middleware support to them.

This change was tested by adding destination filters to an action destination and making sure events and properties were dropped properly.

[x] I've included a changeset (psst. run yarn changeset. Read about changesets here).

changeset-bot · 2022-09-15T16:43:01Z

🦋 Changeset detected

Latest commit: 91f0d8e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@segment/analytics-next	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

chrisradek

Overall I think this looks great, excited to see this middleware support added to action destinations!

Things I wanted to check:

We may need to move alternativeNames to the plugin interface then include it in our checks here:

analytics-next/packages/browser/src/core/queue/event-queue.ts

Lines 236 to 239 in 9c93579

    
           return ( 
        
             denyList[p.name] ?? 
        
             (p.name === 'Segment.io' ? true : denyList.All) !== false 
        
           )

This way if someone is currently disabling/enabling based on the current plugin name (like FullStory (actions) trackEvent) it still works.

Can we apply the above logic for logging/metrics as well?
I commented that we should only apply destination filters on destination plugins - wonder if that should apply for all destination middleware as well. What are your thoughts there given action destinations can have non-destination actions.

.changeset/silver-mugs-provide.md

packages/browser/src/plugins/ajs-destination/index.ts

packages/browser/src/plugins/remote-loader/index.ts

silesky · 2022-09-26T20:16:43Z

packages/browser/src/plugins/remote-loader/index.ts

+
+    return new Context(modifiedEvent)
+  }
+


Is there an opportunity here to be more DRY and shave off some bytes?

Something like:

private _createMethod(methodName: 'track' | 'page' | 'identify' | 'alias' | 'group' | 'screen') { return async (ctx: Context): Promise<Context> => { if (!this.action[methodName]) return ctx const transformedContext = await this.transform(ctx) await this.action[methodName](transformedContext) return ctx } } alias = _createMethod('track') group = _createMethod('group') ... etc

danieljackins · 2022-09-28T20:28:58Z

Overall I think this looks great, excited to see this middleware support added to action destinations!

Things I wanted to check:

We may need to move alternativeNames to the plugin interface then include it in our checks here:

analytics-next/packages/browser/src/core/queue/event-queue.ts

Lines 236 to 239 in 9c93579

return (

denyList[p.name] ??

(p.name === 'Segment.io' ? true : denyList.All) !== false

)

This way if someone is currently disabling/enabling based on the current plugin name (like FullStory (actions) trackEvent) it still works.

Can we apply the above logic for logging/metrics as well?

I commented that we should only apply destination filters on destination plugins - wonder if that should apply for all destination middleware as well. What are your thoughts there given action destinations can have non-destination actions.

Done!
Not exactly sure what you're getting at here
I think it would be safer to not have that restriction just in case they're trying to use them on 'enrichment' actions or something else

chrisradek · 2022-09-28T21:16:24Z

Not exactly sure what you're getting at here

The context keeps logs/stats about which plugins were ran. Currently you can see which actions are ran and some stats about them. With these changes I think we'll see the wrapper plugin's name instead, so Fullstory instead of Fullstory (actions) trackEvent - also makes it look like we're running the same plugin multiple times if a destination has more than 1 action.

from our dev environment:

chrisradek · 2022-09-28T21:20:52Z

packages/browser/src/plugins/remote-loader/index.ts

+            if (
+              routing.length &&
+              routingMiddleware &&
+              plugin.type === 'destination'


Might be worth adding a comment that we only want to apply destination filters to destination actions so that we don't cause issues for hybrid action destinations (where we simply augment the event and send it to the cloud rather than directly to the destination)

I see future selves forgetting why we limited this 😄

silesky · 2022-09-29T05:17:34Z

packages/browser/src/plugins/remote-loader/index.ts

+  }
+
+  unload(ctx: Context, analytics: Analytics): Promise<unknown> | unknown {
+    return this.action.unload && this.action.unload(ctx, analytics)


just in case you aren’t a Typescript nerd, you can shorten this to “return this.action.unload?.(ctx, analytics)” IIRC

silesky · 2022-09-29T05:20:42Z

packages/browser/src/core/queue/event-queue.ts

@@ -232,9 +232,16 @@ export class EventQueue extends Emitter {
        return true
      }

+      const alternativeNameMatch =
+        p.alternativeNames &&
+        p.alternativeNames.filter((name) =>


Optional changing nitpick! p.alternativeNames?.filter would be a bit nicer IMO

I typically agree - though the size savings are not insignificant due to the transpilations that occur (assuming variables/field names get minified). See this as an example. (Just a tradeoff - if it makes the code more readable then go with the optional chaining - I was just surprised how much larger the transpiled code was)

interesting... that's a nice data point. I think I still prefer the optional chain... I wonder if babel would do a better job?

We use optional chaining all over, so IMO we should just use it and not care about size and hope the tooling gets better (or attempt to apply more optimization). or, if we're going to ban it, we should talk about it and then ban it and do a global refactor and use a lint rule to enforce. But If we're going to use it once I'd prefer to use it liberally for readability and then at some future point we target it for cost savings.

silesky · 2022-10-04T19:27:08Z

packages/browser/src/plugins/remote-loader/index.ts

+  private async transform(ctx: Context): Promise<Context> {
+    const modifiedEvent = await applyDestinationMiddleware(
+      this.name,
+      klona(ctx.event),


I wasn't aware that applyDestinationMiddleware mutated the event

Can we avoid that? (If so, we'd want to clone the event inside of the applyDestinationMiddleware function, right?)

Are there any tests that would fail if klona wasn't included?

This is the kind of code smell stuff that I think can get confusing later if the why is not documented (in tests, comments, etc).

We do need to mutate the event I believe, but I moved the cloning logic into applyDestinationMiddleware and added a test that will fail if the original context gets modified (ie. if klona gets removed) 👍

…-next into action-filters

chrisradek

Overall looks great! Just had 1 question that might be an issue.

packages/browser/src/browser/index.ts

chrisradek · 2022-10-12T00:47:13Z

packages/browser/src/core/queue/event-queue.ts

      // Explicit integration option takes precedence, `All: false` does not apply to Segment.io
      return (
        denyList[p.name] ??
+        alternativeNameMatch ??


Isn't alternativeNameMismatch an array of strings here? I think that'd be evaluated as a truthy value so we'd never hit the next conditional, or am I missing something?

Nice catch - fixed the logic and added a new test

Co-authored-by: Christopher Radek <14189820+chrisradek@users.noreply.github.com>

chrisradek

Looks good to me!

danieljackins added 3 commits September 13, 2022 01:24

Wrap actions in plugin to support middleware

f9ab178

Add unit test

1fa9181

add changeset

c6941c6

danieljackins added 6 commits September 20, 2022 15:39

Use creation names

928bcf4

Refactor tsub middleware to be conditional

610b8d9

Fix tests around creationName

7cf9f61

Fix more tests

3a0a0d6

Bump size limit

ec805f0

Add missing creation names

1c09bcb

danieljackins requested review from chrisradek and a team September 22, 2022 15:21

chrisradek reviewed Sep 26, 2022

View reviewed changes

silesky reviewed Sep 26, 2022

View reviewed changes

danieljackins added 3 commits September 28, 2022 14:01

Cleanup from PR feedback, filter based on alternativeNames

9f85e2d

removee unused import

55b2204

Extract klona to its own module

75696fb

chrisradek reviewed Sep 28, 2022

View reviewed changes

silesky reviewed Sep 29, 2022

View reviewed changes

danieljackins added 2 commits September 29, 2022 12:36

log action names

fb5f145

re-lower size limit

50f98e3

silesky reviewed Oct 4, 2022

View reviewed changes

danieljackins added 4 commits October 11, 2022 16:08

move cloning logic to dest middleware, add unit test

24d6a34

Merge branch 'master' into action-filters

9fc4388

increase size a tiny bit

2897b0e

Merge branch 'action-filters' of ssh://github.com/segmentio/analytics…

d324c8e

…-next into action-filters

danieljackins requested a review from silesky October 11, 2022 22:26

danieljackins requested a review from chrisradek October 11, 2022 22:26

chrisradek reviewed Oct 12, 2022

View reviewed changes

danieljackins and others added 2 commits October 12, 2022 10:54

Update packages/browser/src/browser/index.ts

18aa37e

Co-authored-by: Christopher Radek <14189820+chrisradek@users.noreply.github.com>

Fix alternative name logic and add unit test

308dd54

chrisradek approved these changes Oct 12, 2022

View reviewed changes

remove unused import

91f0d8e

danieljackins merged commit 18dc5b0 into master Oct 12, 2022

danieljackins deleted the action-filters branch October 12, 2022 21:28

github-actions bot mentioned this pull request Oct 12, 2022

Version Packages #624

Merged

arielsilvestri mentioned this pull request Nov 17, 2022

Adds integration test for action destinations in addDestinationMiddleware #683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support destination filters for device mode action destinations #597

Support destination filters for device mode action destinations #597

danieljackins commented Sep 15, 2022 •

edited

Loading

changeset-bot bot commented Sep 15, 2022 •

edited

Loading

chrisradek left a comment

silesky Sep 26, 2022 •

edited

Loading

danieljackins Sep 28, 2022

danieljackins commented Sep 28, 2022

chrisradek commented Sep 28, 2022

chrisradek Sep 28, 2022

silesky Sep 29, 2022

silesky Sep 29, 2022

chrisradek Sep 29, 2022

silesky Sep 29, 2022 •

edited

Loading

silesky Oct 4, 2022 •

edited

Loading

danieljackins Oct 11, 2022

chrisradek left a comment

chrisradek Oct 12, 2022

danieljackins Oct 12, 2022

chrisradek left a comment

	return (
	denyList[p.name] ??
	(p.name === 'Segment.io' ? true : denyList.All) !== false
	)

Support destination filters for device mode action destinations #597

Support destination filters for device mode action destinations #597

Conversation

danieljackins commented Sep 15, 2022 • edited Loading

changeset-bot bot commented Sep 15, 2022 • edited Loading

🦋 Changeset detected

chrisradek left a comment

Choose a reason for hiding this comment

silesky Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieljackins commented Sep 28, 2022

chrisradek commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silesky Sep 29, 2022 • edited Loading

Choose a reason for hiding this comment

silesky Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisradek left a comment

Choose a reason for hiding this comment

danieljackins commented Sep 15, 2022 •

edited

Loading

changeset-bot bot commented Sep 15, 2022 •

edited

Loading

silesky Sep 26, 2022 •

edited

Loading

silesky Sep 29, 2022 •

edited

Loading

silesky Oct 4, 2022 •

edited

Loading