Implement the error reporting logic through the AMP Viewer #12986

raywainman · 2018-01-23T21:45:25Z

Implement the error reporting logic through the AMP Viewer

Passes errors to the AMP Viewer if the following is true:
-The viewer is in single document mode
-The viewer is trusted
-The viewer has the errorReporter capability
-The document is opted-in for error interception, this can be done by adding an additional attribute called allow-error-reporting-to-viewer in the <html> tag.

The client embedding the AMP viewer can then use these errors for their own monitoring or reporting.

The error passed to the viewer is the same as the error passed to the AMP error server.

raywainman · 2018-01-23T22:00:56Z

/cc @zhangsu

zhangsu · 2018-01-23T23:22:57Z

/cc @choumx

dreamofabear

Just a few minor comments. Thanks for contributing this!

dreamofabear · 2018-01-25T21:01:56Z

src/error.js

+      return Promise.resolve();
+    }
+    viewer.sendMessage('error', data);
+    return Promise.resolve(true);


return true;

dreamofabear · 2018-01-25T21:02:17Z

src/error.js

+ *
+ * @param {!JsonObject} data Data from `getErrorReportData`.
+ * @return {!Promise<boolean|undefined>} True if the error was sent to the
+ *     viewer, `Promise<undefined>` otherwise.


How about resolve with false to make this signature a bit simpler?

dreamofabear · 2018-01-25T23:11:37Z

src/error.js

+ *     viewer, `Promise<undefined>` otherwise.
+ * @visibleForTesting
+ */
+export function maybeReportErrorToViewer(data) {


Minor: Can we add the window as a param and pass it in through the caller (which uses this instead of self)?

maybeReportErrorToViewer(win, data)

Good call, done.

dreamofabear · 2018-01-25T23:12:50Z

src/error.js

+  const ampdocSingle = ampdocService.getAmpDoc();
+  const htmlElement = ampdocSingle.getRootNode().documentElement;
+  const docOptedIn =
+      htmlElement.hasAttribute('allow-error-reporting-to-viewer');


Bike-shed: report-errors-to-viewer? I'll also solicit suggestions in our weekly communal bike-shed tribunal. 😄

hehe sounds good, I like report-errors-to-viewer!

dreamofabear · 2018-01-25T23:14:56Z

src/error.js

+  }
+
+  const viewer = Services.viewerForDoc(ampdocSingle);
+  if (!viewer.hasCapability('errorReporter')) {


Tangent: I don't see this updated in the internal viewer docs yet.

Right this is ongoing on our end. Dropped you a note about this.

dreamofabear · 2018-01-25T23:15:21Z

src/error.js

+
+  return viewer.isTrustedViewer().then(viewerTrusted => {
+    if (!viewerTrusted) {
+      return Promise.resolve();


Minor: Just directly return the value here.

raywainman · 2018-01-26T20:07:10Z

src/error.js

+ *
+ * @param {!JsonObject} data Data from `getErrorReportData`.
+ * @return {!Promise<boolean|undefined>} True if the error was sent to the
+ *     viewer, `Promise<undefined>` otherwise.


raywainman · 2018-01-26T20:09:47Z

src/error.js

+ *     viewer, `Promise<undefined>` otherwise.
+ * @visibleForTesting
+ */
+export function maybeReportErrorToViewer(data) {


Good call, done.

raywainman · 2018-01-26T20:10:04Z

src/error.js

+  const ampdocSingle = ampdocService.getAmpDoc();
+  const htmlElement = ampdocSingle.getRootNode().documentElement;
+  const docOptedIn =
+      htmlElement.hasAttribute('allow-error-reporting-to-viewer');


hehe sounds good, I like report-errors-to-viewer!

raywainman · 2018-01-26T20:11:18Z

src/error.js

+
+  return viewer.isTrustedViewer().then(viewerTrusted => {
+    if (!viewerTrusted) {
+      return Promise.resolve();


raywainman · 2018-01-26T20:21:50Z

src/error.js

+      return Promise.resolve();
+    }
+    viewer.sendMessage('error', data);
+    return Promise.resolve(true);


raywainman · 2018-01-26T20:54:44Z

src/error.js

+  }
+
+  const viewer = Services.viewerForDoc(ampdocSingle);
+  if (!viewer.hasCapability('errorReporter')) {


Right this is ongoing on our end. Dropped you a note about this.

dreamofabear · 2018-01-26T21:14:38Z

validator/testdata/feature_tests/root_element_attributes.html

@@ -15,7 +15,7 @@
  This demonstrates that certain attributes are not allowed on the root <html> element.
 -->
 <!doctype html>
-<html ⚡ allow-xhr-interception>
+<html ⚡ allow-xhr-interception allow-error-reporting-to-viewer>


report-errors-to-viewer

Done, sorry about that.

dreamofabear · 2018-01-26T21:14:53Z

validator/testdata/feature_tests/root_element_attributes.out

@@ -16,7 +16,9 @@ FAIL
 |    This demonstrates that certain attributes are not allowed on the root <html> element.
 |  -->
 |  <!doctype html>
-|  <html ⚡ allow-xhr-interception>
+|  <html ⚡ allow-xhr-interception allow-error-reporting-to-viewer>


report-errors-to-viewer here and below.

raywainman

Thanks!

raywainman · 2018-01-26T21:54:30Z

validator/testdata/feature_tests/root_element_attributes.out

@@ -16,7 +16,9 @@ FAIL
 |    This demonstrates that certain attributes are not allowed on the root <html> element.
 |  -->
 |  <!doctype html>
-|  <html ⚡ allow-xhr-interception>
+|  <html ⚡ allow-xhr-interception allow-error-reporting-to-viewer>


raywainman · 2018-01-26T21:54:31Z

validator/testdata/feature_tests/root_element_attributes.html

@@ -15,7 +15,7 @@
  This demonstrates that certain attributes are not allowed on the root <html> element.
 -->
 <!doctype html>
-<html ⚡ allow-xhr-interception>
+<html ⚡ allow-xhr-interception allow-error-reporting-to-viewer>


Done, sorry about that.

…t#12986) * error reporting to the AMP viewer * add trusted viewer checks for the error reporting * address error log pull review comments * remove redundant line in error test * validator feature test for new allow-error-reporting-to-viewer attribute * fix presubmit errors * address pull request comments fo the error reporting capability * update testdata files to reflect new capability

raywainman added 6 commits January 22, 2018 09:14

error reporting to the AMP viewer

0e0976f

add trusted viewer checks for the error reporting

46a2018

address error log pull review comments

32e5c3f

remove redundant line in error test

e3cfe8b

validator feature test for new allow-error-reporting-to-viewer attribute

a3e1db0

fix presubmit errors

3623b6c

googlebot added the cla: yes label Jan 23, 2018

dreamofabear requested review from jridgewell and dreamofabear January 23, 2018 23:36

dreamofabear approved these changes Jan 25, 2018

View reviewed changes

address pull request comments fo the error reporting capability

a1139a0

raywainman commented Jan 26, 2018

View reviewed changes

dreamofabear reviewed Jan 26, 2018

View reviewed changes

update testdata files to reflect new capability

e12fb45

raywainman commented Jan 26, 2018

View reviewed changes

dreamofabear merged commit 2cdc9d3 into ampproject:master Jan 31, 2018

ampprojectbot added the PR use: In Stable label Feb 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the error reporting logic through the AMP Viewer #12986

Implement the error reporting logic through the AMP Viewer #12986

raywainman commented Jan 23, 2018 •

edited by dreamofabear

raywainman commented Jan 23, 2018

zhangsu commented Jan 23, 2018

dreamofabear left a comment

dreamofabear Jan 25, 2018

raywainman Jan 26, 2018

dreamofabear Jan 25, 2018

dreamofabear Jan 25, 2018

raywainman Jan 26, 2018

dreamofabear Jan 25, 2018

raywainman Jan 26, 2018

dreamofabear Jan 25, 2018

raywainman Jan 26, 2018

dreamofabear Jan 25, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

raywainman Jan 26, 2018

dreamofabear Jan 26, 2018

raywainman Jan 26, 2018

dreamofabear Jan 26, 2018

raywainman Jan 26, 2018

raywainman left a comment

raywainman Jan 26, 2018

raywainman Jan 26, 2018

Implement the error reporting logic through the AMP Viewer #12986

Implement the error reporting logic through the AMP Viewer #12986

Conversation

raywainman commented Jan 23, 2018 • edited by dreamofabear

Implement the error reporting logic through the AMP Viewer

raywainman commented Jan 23, 2018

zhangsu commented Jan 23, 2018

dreamofabear 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

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

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

raywainman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raywainman commented Jan 23, 2018 •

edited by dreamofabear