CORE-51982 Use sendBeacon for link tracking. by Aaronius · Pull Request #598 · adobe/alloy

Aaronius · 2020-09-25T21:00:58Z

Description

We weren't using sendBeacon for click tracking and now we are.

Related Issue

https://jira.corp.adobe.com/browse/CORE-51982

Motivation and Context

We want to be using sendBeacon on link clicks because there's a high likelihood the user is navigating away from the page and we want to ensure the beacon reaches the server.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (non-breaking change which does not add functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have signed the Adobe Open Source CLA or I'm an Adobe employee.
I have made any necessary test changes and all tests pass. ** Some integration tests are failing but should be fixed by d4e7cc2
I have run the Sandbox successfully.

Aaronius · 2020-09-25T21:03:07Z

sandbox/src/LargePayload.js

      <h2>Large Payloads</h2>
      <p>This page tests send really large payloads to the edge.</p>
      <p>
-        All those requests should not use beacon calls, and should not fail.


This wasn't accurate. The command options include documentUnloading: true, which means Alloy does use sendBeacon. Depending on the browser, some payload will fail to be sent using sendBeacon and Alloy will fall back to fetch.

Wouldn't this be configured without documentUnloading to avoid sendBeacon limit then?

I'm not sure I understand your question, but this page is to test what happens when sendBeacon is used, so I believe the configuration (documentUnloading: true) is correct, but this description we had previously didn't make sense and didn't correlate with our configuration.

Aaronius · 2020-09-25T21:18:03Z

src/core/edgeNetwork/requestPayloads/createDataCollectionRequestPayload.js

      addEvent(event) {
        content.events = content.events || [];
-        content.events.push(event.toJSON());
+        content.events.push(event);


This change here probably deserves some explanation. I believe we were calling event.toJSON() here because when we log the payload to the console, we don't want the event's methods and stuff showing up on the logged object. Since the time this was written, we've adjusted how we log the payload to the console to be smarter: https://github.com/adobe/alloy/blob/master/src/core/network/injectSendNetworkRequest.js#L24-L39

When we log the payload, we serialize and then deserialize the object. When the object is serialized, JSON.parse recursively goes through the payload object, including its events, calling toJSON. Because of this, there's no reason to do event.toJSON() here anymore. We also don't want to do event.toJSON() here anymore because we need access to its methods in the new getDocumentMayUnload method below.

This also affects when the lastChanceCallback gets called. The cloning of the object is also important so that if the event is queued, when the event is sent, it contains the data as it was when sendEvent was called. We should probably break out some of the toJSON code into a "freeze" method.

Thanks for pointing this out. I thought I might be missing something. So, with the changes in this PR, the last chance callback would get called when the payload gets serialized, which seems to me to be more appropriate than the way it was previously, since the callback will now be called be as late as possible.

Regarding cloning of customer data, I feel like it should be the responsibility of the data collector to clone the data ASAP, even before it makes its way into the event.

Thoughts on that?

Pros of the approach in this PR:

using the lastChanceCallback to set the consent and preferences on pre-consent events is possible

Customers have the flexibility of giving an object to sendEvent that can change up until the event is serialized or providing a copy

Cons:

Customers may be confused about when the xdm and data objects are frozen. (We can solve this with documentation)

I'm sold on the change in this PR.

Aaronius · 2020-09-25T21:18:59Z

src/core/network/injectSendNetworkRequest.js

+      return networkStrategy({
+        url,
+        body: stringifiedPayload,
+        documentMayUnload: payload.getDocumentMayUnload()


This is the crux of why sendBeacon wasn't being used before. We weren't passing documentMayUnload.

Aaronius · 2020-09-25T21:19:15Z

test/functional/helpers/constants/testServerUrl.js

  } else {
    pageUrl = alloyPages[env];
  }
+  // eslint-disable-next-line no-console


I was seeing lint warnings.

Aaronius · 2020-09-25T21:20:30Z

test/unit/specs/components/Identity/getIdentity/createIdentityPayload.spec.js

    payload.addIdentity("IDNS", {
      id: "ABC123"
    });
-    expect(payload.toJSON()).toEqual({


This technically wasn't really serializing. JSON.stringify recursively calls toJSON on the content passed to it, which is a better representation of what we're trying to test.

jonsnyder · 2020-09-25T21:45:16Z

src/core/edgeNetwork/requestPayloads/createDataCollectionRequestPayload.js

      addEvent(event) {
        content.events = content.events || [];
-        content.events.push(event.toJSON());
+        content.events.push(event);


This also affects when the lastChanceCallback gets called. The cloning of the object is also important so that if the event is queued, when the event is sent, it contains the data as it was when sendEvent was called. We should probably break out some of the toJSON code into a "freeze" method.

…con call detection.

Aaronius · 2020-10-02T16:24:07Z

src/core/createEvent.js

-        }
-        if (content.data || !isEmptyObject(data)) {
-          content.data = data;
+      if (lastChanceCallback) {


This modification was to limit the amount we're cloning xdm and data.

Aaronius · 2020-10-02T16:32:49Z

src/components/DataCollector/index.js

+          if (xdm) {
+            // Clone xdm in case the customer modifies the object
+            // between now and the time it gets sent to the server.
+            event.setUserXdm(clone(xdm));


Like the comment says, DataCollector is now cloning xdm and data as soon as possible so that if the customer modifies the object between now and the time it gets sent to the server, we use the original state and not the state after modification. What's interesting is that the command option validator we're using (which leverages Yup) also clones this data in some fashion, making this particular clone call unnecessary to accomplish our goal, but yet I prefer that we explicitly clone here anyway so that our intentions are explicit and we're not relying on the validator to do the cloning.

liljenback · 2020-10-12T21:26:55Z

sandbox/src/EventMerge.js


  useEffect(() => {
-    eventMergeIdPromise.current.then(eventMergeId => {
+    eventMergeIdPromise.current.then(result => {


This is a separate fix for something else?

Yeah, I thought I left a comment somewhere but I don't know where it is now. The event merge ID view in the sandbox was failing because the promise is resolved with an object containing the event merge ID rather than the event merge ID itself.

jfkhoury

👍 💯

Bumps [ajv](https://github.com/ajv-validator/ajv) from 8.17.1 to 8.18.0. - [Release notes](https://github.com/ajv-validator/ajv/releases) - [Commits](ajv-validator/ajv@v8.17.1...v8.18.0) --- updated-dependencies: - dependency-name: ajv dependency-version: 8.18.0 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

CORE-51982 Use sendBeacon for link tracking.

cb4afd6

Aaronius commented Sep 25, 2020

View reviewed changes

jonsnyder requested changes Sep 25, 2020

View reviewed changes

Aaronius added 3 commits October 1, 2020 14:36

CORE-51982 Clone xdm and data options as soon as possible. Added tests.

6b6b6d9

Merged master into send-beacon. Resolved conflict in testServerUrl.js.

c209166

CORE-51982 Fixed intermittent functional test failures due to sendBea…

c4a575c

…con call detection.

Aaronius commented Oct 2, 2020

View reviewed changes

CORE-51982 Fixing test description.

00731c6

Aaronius requested a review from jonsnyder October 2, 2020 16:38

CORE-51982 Removed cloning of user-provided data.

c937207

jonsnyder approved these changes Oct 2, 2020

View reviewed changes

liljenback reviewed Oct 12, 2020

View reviewed changes

jfkhoury approved these changes Oct 19, 2020

View reviewed changes

Aaronius merged commit a519a7c into master Oct 19, 2020

Aaronius deleted the send-beacon branch October 19, 2020 16:28

Conversation

Aaronius commented Sep 25, 2020

Description

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

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!

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!

jfkhoury left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants