[WIP] Move Google Analytics and Sentry to hooks that apps can call #196

dmethvin-gov · 2018-07-30T18:36:05Z

Fixes #134

Putting this up for discussion now since there are some thorny questions about back-compat with the existing vets.gov--see comments.

The basic idea is to move the GA and error reporting to the app rather than embed it in the library. This gives the app writer the opportunity to decide what should be reported and which one of the dozens of event reporter services they should use. I've put the hook in formConfig.clientTelemetry but better names are welcome. The hook function does not need to be passed the formConfig because the app writer's hook function can always get it directly from their own project or use formConfig.clientTelemetry.bind(formConfig) if they prefer.

Once design issues are resolved this PR still needs:

Docs on the feature as finally implemented
Unit tests

And separately in us-forms-system-starter-app

Skeletal implementation, using console.error or similar perhaps?

dmethvin-gov · 2018-07-30T18:37:44Z

package.json

@@ -28,19 +28,19 @@
  },
  "homepage": "https://github.com/usds/us-forms-system#readme",
  "devDependencies": {


An npm update slipped in here, I can back it out if needed. The only related change here is that raven-js was removed. If the app wanted it they would name it directly as a dependency.

dmethvin-gov · 2018-07-30T18:40:02Z

src/js/actions.js

-    });
+  const telemetry = (category, error) => {
+    if (formConfig.clientTelemetry) {
+      formConfig.clientTelemetry(category, error);


This clientTelemetry call can do both a Sentry and GA.

dmethvin-gov · 2018-07-30T18:43:17Z

src/js/actions.js

-        }
-        captureError(error, errorType);
-        dispatch(setSubmission('status', errorType, error.extra));
+        telemetry('error', error);


These are obviously errors specific to vets.gov, so I pulled this out. If vets.gov needs site-specific messages they could be sent by the hook function. (Essentially, move the deleted code to the hook function I think,)

dmethvin-gov · 2018-07-30T18:46:19Z

src/js/actions.js

@@ -241,7 +225,6 @@ export function uploadFile(file, uiOptions, onProgress, onChange, onError) {
          name: file.name,
          errorMessage
        });
-        Raven.captureMessage(`vets_upload_error: ${req.statusText}`);


This code doesn't currently have access to formConfig as an argument, but in addition the error is vets.gov-specific so it seems like this needs to be isolated some way. Also, we don't have file uploads implemented anyway so this code doesn't run and can't be tested.

Should we just move file upload out of our library until we can build it in a generally usable way?

Probably. It's super useful functionality but we need it to be generalized so it can work with other servers.

annekainicUSDS · 2018-08-01T23:24:18Z

This is interesting so far, and definitely a good idea to find a way to pull these events out of the library. What I don't understand right now is how the information being logged to clientTelemetry gets sent back to the app?

dmethvin-gov · 2018-08-02T00:41:13Z

That would be the job of the app. Per that last checkbox I figured we could add a sample hook to the starter app that just logged to the console. I don't think we should actually try to put real telemetry in the sample app, it requires the app-writer's Google Analytics or error reporting provider's number.

Looking back through the library, we don't really log a lot here. I think it would be interesting to fire the callback each time the person navigates to a page so it would be possible to get form-abandon stats. This isn't data that vets.gov is expecting so it would have to be filtered out in their app-specific hook when they upgraded to this version.

dmethvin-gov · 2018-08-28T19:56:09Z

I'm going to wait until #209 lands because this one touches nearby areas and will need to be fixed.

dmethvin-gov · 2018-09-04T19:23:27Z

src/js/actions.js

@@ -242,7 +229,8 @@ export function uploadFile(file, uiOptions, onProgress, onChange, onError) {
          name: file.name,
          errorMessage
        });
-        Raven.captureMessage(`vets_upload_error: ${req.statusText}`);
+        // Commenting until this is removed by a PR for #211


We'll be pulling out the vets-specific file upload code when #211 is fixed but I didn't want to start unraveling it here.

dmethvin-gov · 2018-09-04T19:25:00Z

src/js/actions.js

-  };
+  const recordEvent = formConfig.recordEvent ?
+    formConfig.recordEvent.bind(formConfig) :
+    console.log.bind(console);      // eslint-disable-line no-console


It seemed bad to be totally silent when some of these things happened so I figured we could put them on the console. If someone is upset because the console has been soiled and they don't want the messages, they can define a function that does nothing. 😨

I feel a little weary of publishing a bunch of stuff to the console, but I don't really know what's best practice here. It seems like warnings that are useful for development make sense to be logged to the console, but I'm not aware of any apps that log user-created events to the console. Are there risks with this?

dmethvin-gov · 2018-09-04T19:27:42Z

src/js/review/SubmitController.jsx

-        }
-      });
+      const recordEvent = formConfig.recordEvent ?
+        formConfig.recordEvent.bind(formConfig) :


Yes this is a duplicate of the code in SubmitController. I was thinking that we might not want to log some messages that I want to add in the future (e.g. { event: 'user-changed-pages', from: 2, to: 3 }) so it seemed bad to overwrite formConfig.recordEvent with a global default.

dmethvin-gov · 2018-09-04T19:28:39Z

src/js/helpers.js

@@ -578,13 +578,3 @@ export function omitRequired(schema) {

  return newSchema;
 }
-
-/**
- * Helper function for reporting events to Google Analytics. An alias for window.dataLayer.push.


If an app wants to use GA they'd do that in their app.

dmethvin-gov · 2018-09-04T19:29:35Z

src/js/state/helpers.js

@@ -463,8 +463,7 @@ export function createInitialState(formConfig) {
    reviewPageView: {
      openChapters: [],
      viewedPages: new Set()
-    },
-    trackingPrefix: formConfig.trackingPrefix


We don't need trackingPrefix anymore since that data would be held by the app and depend on what service they used.

I'm confused how this part would work. It seems like they may still want a way to differentiate between events of the same name that happen on different forms, so it seems like this would still be needed in our code when we log events?

dmethvin-gov · 2018-09-04T19:31:55Z

I think this is ready for another review pass.

This version has docs for the feature, I was unsure of where to put them.

bernars-usa

Just minor comments from me!

bernars-usa · 2018-09-05T14:08:25Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

+
+The `recordEvent` function receives a single object that can contain different information depending on the kind of event being reported. Events reported by the library always have a `event` property that is a string describing the event. The events currently supported by the library are:
+* **validation-failed-on-submit**: The user completed the form and tried to submit it, but there were still validation errors. This is most likely an error in the form definition or one of the React components, since the form should already be fully validated by the time this point is reached.
+* **form-submit-pending**: The user has pressed the submit button, the form validated, and the form has been sent to the server. This is informational only and does not represent an error.


The user has pressed the submit button, the form validated, and the form has been sent to the server. This is informational only and does not represent an error.

Maybe:

"An informational message showing that the user has pressed the submit button, and that the form has validated and been sent to the server."

bernars-usa · 2018-09-05T14:10:16Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

+* **validation-failed-on-submit**: The user completed the form and tried to submit it, but there were still validation errors. This is most likely an error in the form definition or one of the React components, since the form should already be fully validated by the time this point is reached.
+* **form-submit-pending**: The user has pressed the submit button, the form validated, and the form has been sent to the server. This is informational only and does not represent an error.
+* **form-submit-successful**: The server returned a status indicating it has accepted the form.
+* **form-submit-error**: The form was submitted but some problem occurred that prevented it from being accepted by the server. The object contains an `error` and `errorType` with more information about the nature of the error.


The form was submitted but some problem occurred that prevented it from being accepted by the server. The object contains an error and errorType with more information about the nature of the error.

How about:

"The form was submitted, but the server didn't accept the submission. The object contains an error and errorType with more information about the nature of the error."

bernars-usa · 2018-09-05T14:18:56Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

+If you provide a function for `formConfig.recordEvent`, the library calls that function when notable events occur. If you do not provide a function, the library logs these events onto the browser console.
+
+The `recordEvent` function receives a single object that can contain different information depending on the kind of event being reported. Events reported by the library always have a `event` property that is a string describing the event. The events currently supported by the library are:
+* **validation-failed-on-submit**: The user completed the form and tried to submit it, but there were still validation errors. This is most likely an error in the form definition or one of the React components, since the form should already be fully validated by the time this point is reached.


This is most likely an error in the form definition or one of the React components, since the form should already be fully validated by the time this point is reached.

Can we get away with:

"This is likely an error in the form definition or a React component, since the form should be completely validated at this point."

annekainicUSDS

Overall the direction makes sense to me! Some questions.

annekainicUSDS · 2018-09-06T13:47:38Z

src/js/actions.js

-        recordEvent({
-          event: `${trackingPrefix}-submission-successful`,
-        });
+        recordEvent({ event: 'form-submit-successful' });


The tracking prefix that existed before in the event name was useful for Vets.gov because there are so many forms that they need to be able to know which one specifically was successful. I assume this could be the case for other users as well. Do you think we can account for this the way we did before, by including a key in the formConfig for trackingPrefix? Or is there a better way? Maybe allowing someone to customize the message that gets passed for various events?

The function that gets called is provided by the app (in that case, the vets.gov website) so they can make whatever changes they'd like for each event, and it can be customized per event. We don't need to incorporate any values in the formConfig since they can add whatever they want in their function. One thing that I did which may be worth reconsidering is I used .bind() to bind the function to the formConfig, but on second thought it makes more sense to have the creator of form.js do that if that's what they want. It's possible they might want to pass in a bound "EventReportingClass" method that encapsulates whatever data they need including trackingPrefix.

annekainicUSDS · 2018-09-06T13:56:05Z

src/js/actions.js

@@ -177,7 +164,7 @@ export function submitForm(formConfig, form) {
        } else if (errorMessage.startsWith('vets_server_error')) {
          errorType = 'serverError';
        }
-        captureError(error, errorType);
+        recordEvent({ event: 'form-submit-error', error, errorType });


So is this new event, form-submit-error replacing the submission-failed event we previously had? It makes me wonder about current users of the library that the names of the logged errors will be changing, and if that will create a downstream problem for them in tracking issues. I wonder if we should enable someone to customize the names of the events that get logged.

These changes have to occur in a major version release so they're breaking changes regardless. Most likely the only client using this logic at the moment is vets.gov because of the very specific configuration and messages (e.g., vets_server_error above). In their recordEvent setup they can just map form-submit-error to submission-failed if they want to keep the same name. I just wanted to make the names consistent and since these three were about the form submit process I used the same prefix for their names.

annekainicUSDS · 2018-09-06T13:59:26Z

src/js/review/SubmitController.jsx

+        formConfig.recordEvent.bind(formConfig) :
+        console.log.bind(console);   // eslint-disable-line no-console
+
+      recordEvent({ event: 'validation-failed-on-submit', errors });


This makes me wonder about where these events should be logged. Below we're also dispatching a redux action for the submission status, so there are 2 places here where information is being communicated out about the same event. Should we consolidate this somehow? Should the events be logged from the updated state? I'm not sure, just opening it as a possible discussion point :)

I suspected that duplication was intentional so I kept it. The event recording is as independent as possible to the app logic to avoid being affected by app bugs.

annekainicUSDS · 2018-09-06T14:01:10Z

src/js/state/helpers.js

@@ -463,8 +463,7 @@ export function createInitialState(formConfig) {
    reviewPageView: {
      openChapters: [],
      viewedPages: new Set()
-    },
-    trackingPrefix: formConfig.trackingPrefix


I'm confused how this part would work. It seems like they may still want a way to differentiate between events of the same name that happen on different forms, so it seems like this would still be needed in our code when we log events?

annekainicUSDS · 2018-09-06T14:02:39Z

src/js/actions.js

-  };
+  const recordEvent = formConfig.recordEvent ?
+    formConfig.recordEvent.bind(formConfig) :
+    console.log.bind(console);      // eslint-disable-line no-console


I feel a little weary of publishing a bunch of stuff to the console, but I don't really know what's best practice here. It seems like warnings that are useful for development make sense to be logged to the console, but I'm not aware of any apps that log user-created events to the console. Are there risks with this?

dmethvin-gov · 2018-09-06T14:40:15Z

GitHub is being bizarre today, I can't reply to two (but only two) of Anne's comments so I'll do it here.

I'm confused how this part would work. It seems like they may still want a way to differentiate between events of the same name that happen on different forms, so it seems like this would still be needed in our code when we log events?

The person setting up the form.js can configure whatever they want including unique strings and account numbers to prefix or suffix (or even rename) the event names that we pass back. The starter app can have a stub showing how that's done.

I feel a little weary of publishing a bunch of stuff to the console, but I don't really know what's best practice here. It seems like warnings that are useful for development make sense to be logged to the console, but I'm not aware of any apps that log user-created events to the console. Are there risks with this?

I don't really like spewing on the console either, but except for the form-submit-successful these represent some sort of bug in the program so it seemed worse to be silent about it by default, especially on the production forms. I thought about having some sort of "priority" or "severity" that would separate info/warning/error but wondered if that was overkill. I haven't seen what happens on the screen when the validation-failed-on-submit case occurs but it seems like the user would have no clue at all and no action occurred when they pressed the button.

dmethvin-gov · 2018-09-06T14:44:01Z

Oh @annekainicUSDS maybe one option would be to make formConfig.recordEvent mandatory? That way the app has to do something with the data, even if just to unwisely throw it away. In that case we'd eliminate the console.log on our side.

annekainicUSDS

In general looks good to me! I think there might still be some open questions (logging on the console? making recordEvent mandatory?), but perhaps we get feedback on that once someone (probably Vets.gov) actually starts using this and lets us know what they think.

annekainicUSDS · 2018-09-11T14:31:31Z

docs/building-a-form/available-form-features-and-usage-guidelines.md

+
+### Usage guidelines
+
+Web applications can fail for many reasons, including bad Internet connections, outdated browsers, and misbehaved browser extensions. Even if you test thoroughly, users may experience frustrating errors that they do not report. We highly recommend that you use an error and event reporting service to track the use of your forms. Examples of services that could be used are Google Analytics, Errorception, Sentry, Airbrake, and Raygun.


What do you think about including a code sample to show what this function might look like? Something like:

"Here's an example of what the function passed to formConfig.recordEvent might look like if you were using Google Analytics to track events:

formConfig = { ... recordEvent: (event) => { return window.dataLayer && window.dataLayer.push(event); }, ...

"

Maybe also an example of how they would change the name of the event they log on their side?

…196)" This reverts commit f40fe50.

dmethvin-gov commented Jul 30, 2018

View reviewed changes

dmethvin-gov mentioned this pull request Aug 9, 2018

Update "Example form.js" document #212

Closed

dmethvin-gov force-pushed the 134-tracking branch from 63d3ff0 to 70a9a51 Compare August 28, 2018 19:53

dmethvin-gov force-pushed the 134-tracking branch from 70a9a51 to 8b791e0 Compare September 4, 2018 18:42

dmethvin-gov commented Sep 4, 2018

View reviewed changes

bernars-usa reviewed Sep 5, 2018

View reviewed changes

annekainicUSDS reviewed Sep 6, 2018

View reviewed changes

annekainicUSDS approved these changes Sep 11, 2018

View reviewed changes

dmethvin-gov added 9 commits September 11, 2018 13:51

Rebase on latest master

6be597d

Documentation is sweeping the nation

60141ca

PR feedback on docs

217ea09

Add example to docs

6af66f1

Update built files

b49a710

Remove trackingPrefix prop

8802dd7

Fix typo

0570b16

Fix another typo

1335f13

Last set of typos

04e43f2

dmethvin-gov force-pushed the 134-tracking branch from e673598 to 04e43f2 Compare September 11, 2018 17:52

dmethvin-gov merged commit f40fe50 into usds:master Sep 11, 2018

dmethvin-gov deleted the 134-tracking branch September 11, 2018 17:55

dmethvin-gov mentioned this pull request Sep 18, 2018

Reduce production file size #152

Closed

8 tasks

dmethvin-gov added a commit that referenced this pull request Sep 19, 2018

Revert "Move Google Analytics and Sentry to hooks that apps can call (#…

c30d367

…196)" This reverts commit f40fe50.

dmethvin-gov mentioned this pull request Sep 19, 2018

Create a 1.2.0 release #272

Closed


		### Usage guidelines

		Web applications can fail for many reasons, including bad Internet connections, outdated browsers, and misbehaved browser extensions. Even if you test thoroughly, users may experience frustrating errors that they do not report. We highly recommend that you use an error and event reporting service to track the use of your forms. Examples of services that could be used are Google Analytics, Errorception, Sentry, Airbrake, and Raygun.

[WIP] Move Google Analytics and Sentry to hooks that apps can call #196

[WIP] Move Google Analytics and Sentry to hooks that apps can call #196

Conversation

dmethvin-gov commented Jul 30, 2018

dmethvin-gov Jul 30, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annekainicUSDS commented Aug 1, 2018

dmethvin-gov commented Aug 2, 2018

dmethvin-gov commented Aug 28, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov commented Sep 4, 2018

bernars-usa left a comment

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

annekainicUSDS left a comment

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

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

Choose a reason for hiding this comment

dmethvin-gov commented Sep 6, 2018

dmethvin-gov commented Sep 6, 2018

annekainicUSDS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmethvin-gov Jul 30, 2018 •

edited

Loading