Support destination filters for device mode action destinations by danieljackins · Pull Request #597 · segmentio/analytics-next

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.

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

+
+    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

+            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

+  }
+
+  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


+      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

+  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.

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

      // 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 a team and chrisradek 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

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

Conversation

danieljackins commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silesky Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljackins commented Sep 28, 2022

Uh oh!

chrisradek commented Sep 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silesky Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisradek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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