[STRATCONN-588] Generate bundledConfigIds #555

gpsamson · 2021-02-03T21:43:42Z

What does this PR do?
This pull request builds a list of bundledds based on the intersection between bundled and maybeBundledConfigIds. bundledIds is then appended to each event's metadata object.

The related ajs-renderer change can be found here: https://github.com/segmentio/ajs-renderer/pull/256.

Example metadata object

601c3fef8942cb2ad5d70cdf is the FullStory instance that is not Connection Mode configurable because there is only a browser integration.
601c3c238942cb7cb5d70cde is the Google Analytics instance configured in Device Mode.
601c4f8164b0701d3b6b78cd is the HubSpot instance that is not Connection Mode configurable, but defaults to Device Mode because it's an a.js source.
601c40c46b6df7898d069abf is the Amplitude instance that is configured explicitly in Cloud Mode.

Are there breaking changes in this PR?
No. We specifically are adding new metadata fields to avoid introducing breaking changes to bundled and unbundled.

Testing

Testing completed successfully in stage via end-to-end tests with the related ajs-renderer change.
Existing and new unit tests are passing

Any background context you want to provide?

Is there parity with the server-side/android/iOS integration components (if applicable)?

Does this require a new integration setting? If so, please explain how the new setting works

Links to helpful docs and other external resources

gpsamson · 2021-02-05T08:02:29Z

integrations/segmentio/lib/index.js

+    msg._metadata.bundledConfigIds = bundledConfigIds;
    msg._metadata.unbundled = this.options.unbundledIntegrations;
-    msg._metadata.bundledConfigIds = this.options.bundledConfigIds;
    msg._metadata.unbundledConfigIds = this.options.unbundledConfigIds;


I decided not to ignore empty bundledConfigIds and unbundledConfigIds arrays so it is clear when an event was tracked using this new version of segmentio.

gpsamson · 2021-02-05T08:10:31Z

integrations/segmentio/lib/index.js

 var Queue = require('@segment/localstorage-retry');

-const json = JSON;
+var json = JSON;


This was unintentionally switched to const in #553.

integrations/segmentio/lib/index.js

mericsson · 2021-02-08T19:31:51Z

integrations/segmentio/lib/index.js

+      if (!maybeBundledConfigIds) {
+        break
+      }
+      if (!maybeBundledConfigIds[name]) {


Looks like the casing is the same of destination names in maybeBundledConfigIds and analytics.Integrations e.g. Google Analytics === Google Analytics vs google-analytics.. but perhaps something to double check.

Yup, the casing here is the same but they depend on two different sources.

- `bundled` depends on the name that is defined within the integration code itself: https://github.com/segmentio/analytics.js-integrations/blob/master/integrations/google-analytics/lib/index.js#L37 - `bundledConfigIDs` depends on the defintiion creation name: https://github.com/segmentio/ajs-renderer/pull/256/files#diff-ca733fb7a822baffec24f4f426ccb07fce89f5b4d5856f848be4845d1d40e12bR44

The chance of these being different is unlikely but possible. Fwiw, the integration wouldn't receive the proper settings if the name within the integration code doesn't match the destination creation name because settings are keyed by the definition creation name.

+1 to Gabe's comment, I spot checked a few destinations which I know have undergone name changes and the name defined in integration code marched the definition creation name.

mericsson · 2021-02-08T19:32:49Z

integrations/segmentio/lib/index.js

+
+    // Generate a list of bundled config IDs using the intersection of
+    // bundled destination names and maybe bundled config IDs.
+    var bundledConfigIds = []


I think var bundledConfigIds = [] should be defined outside of his conditional for clarity. This works currently because of use of var vs let/const.

I don't think so -- msg._metadata.bundledConfigIds (L343) is defined within the conditional. I'm also mimicking the scope of bundled.

Co-authored-by: Marcus Ericsson <36717+mericsson@users.noreply.github.com>

gpsamson · 2021-02-08T23:05:49Z

integrations/segmentio/lib/index.js

+  .option('unbundledConfigIds', [])
+  .option('maybeBundledConfigIds', {});


If we were to conditionally add these arrays in ajs-renderer we would still default to empty values here. 🤔

Makes sense. I agree.

briemcnally · 2021-02-09T16:41:22Z

integrations/segmentio/lib/index.js

+      }
+
+      for (var j = 0; j < maybeBundledConfigIds[name].length; j++) {
+        var id = maybeBundledConfigIds[name][j]


Do we maybe want to see if maybeBundledConfigIds[name] has more than one configId? In example case where maybeBundledConfigIds looks like the below, wouldn't we want to collect stats since that would mean they have multiple device mode configs of a destination in device mode, which is not allowed?

maybeBundledConfigIds: { Fullstory: [ config1, config2] }

Added comment here regarding this https://github.com/segmentio/ajs-renderer/pull/256/files#r577105220

gpsamson · 2021-02-09T18:16:52Z

integrations/segmentio/lib/index.js

    msg._metadata.unbundled = this.options.unbundledIntegrations;
-    msg._metadata.bundledConfigIds = this.options.bundledConfigIds;
+    msg._metadata.bundledConfigIds = bundledConfigIds;
    msg._metadata.unbundledConfigIds = this.options.unbundledConfigIds;


unbundledConfigIds is not used in data plane so it could be removed later on.

juliofarah · 2021-02-09T19:47:30Z

integrations/segmentio/package.json

  "name": "@segment/analytics.js-integration-segmentio",
  "description": "The Segmentio analytics.js integration.",
-  "version": "4.3.1",
+  "version": "4.4.1",


nit: should this be 4.4.0 ?

Yup, thanks for catching,

mericsson · 2021-02-16T20:12:09Z

integrations/segmentio/lib/index.js

+    var bundledConfigIds = []
+    for (var i = 0; i < bundled.length; i++) {
+      var name = bundled[i]
+      if (!maybeBundledConfigIds) {


I think it would be clearer if this conditional was outside of the loop since it does not depend on anything in bundled.

We'd have to refactor and wrap the for-loop in a conditional. I can go either way but was trying to avoid nested conditionals.

if (maybeBundledConfigIds) { for (var i = 0; i < bundled.length; i++) { ... } }

mericsson

lgtm - just one code cleanliness issue #555 (comment)

Spike intersection of bundled and maybeBundledConfigIds

3334c3a

gpsamson changed the title ~~Spike intersection of bundled and maybeBundledConfigIds~~ [STRATCONN-588] Generate bundledConfigIds Feb 4, 2021

Gabriel P Samson added 2 commits February 3, 2021 23:13

Update test cases and short-circuit when maybeBundledConfigIds falsy

9e6652b

Fix bad loop + Add test

9d2e22a

gpsamson force-pushed the maybe-bundled branch from e74b839 to 9d2e22a Compare February 5, 2021 05:16

gpsamson marked this pull request as ready for review February 5, 2021 07:59

gpsamson requested review from anoonan, briemcnally and mericsson February 5, 2021 07:59

gpsamson commented Feb 5, 2021

View reviewed changes

Update segmentio version to 4.4.1

7bbbcb4

gpsamson commented Feb 5, 2021

View reviewed changes

mericsson reviewed Feb 8, 2021

View reviewed changes

integrations/segmentio/lib/index.js Outdated Show resolved Hide resolved

mericsson reviewed Feb 8, 2021

View reviewed changes

Update metadata ordering

f9960cc

Co-authored-by: Marcus Ericsson <36717+mericsson@users.noreply.github.com>

gpsamson commented Feb 8, 2021

View reviewed changes

briemcnally reviewed Feb 9, 2021

View reviewed changes

gpsamson commented Feb 9, 2021

View reviewed changes

gpsamson requested review from juliofarah and nettofarah February 9, 2021 19:14

juliofarah requested a review from pooyaj February 9, 2021 19:17

juliofarah reviewed Feb 9, 2021

View reviewed changes

Fix version number

7d544aa

briemcnally approved these changes Feb 10, 2021

View reviewed changes

mericsson reviewed Feb 16, 2021

View reviewed changes

mericsson approved these changes Feb 16, 2021

View reviewed changes

Rename bundledConfigIds + Remove unbundledConfigIds

8569f31

gpsamson requested review from briemcnally and mericsson February 18, 2021 19:30

mericsson approved these changes Feb 18, 2021

View reviewed changes

briemcnally approved these changes Feb 19, 2021

View reviewed changes

gpsamson merged commit a441a1f into master Feb 22, 2021

gpsamson deleted the maybe-bundled branch February 22, 2021 18:45

		.option('unbundledConfigIds', [])
		.option('maybeBundledConfigIds', {});

[STRATCONN-588] Generate bundledConfigIds #555

[STRATCONN-588] Generate bundledConfigIds #555

Uh oh!

Conversation

gpsamson commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpsamson Feb 5, 2021 • 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

briemcnally Feb 9, 2021 • 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!

mericsson Feb 8, 2021 • 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

mericsson left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gpsamson commented Feb 3, 2021 •

edited

Loading

gpsamson Feb 5, 2021 •

edited

Loading

briemcnally Feb 9, 2021 •

edited

Loading

mericsson Feb 8, 2021 •

edited

Loading