Send 1% sampled RTC error pings #14563

bradfrizzell · 2018-04-11T20:24:39Z

With this change, vendors and publishers will be able to get some simple error reporting.

keithwrightbos · 2018-04-13T13:40:25Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+    if (isObject(urlObj)) {
+      url = urlObj['url'];
+      errorReportingUrl = urlObj['errorReportingUrl'];
+    }


else if (typeof urlObj == 'string') url = urlObj;
else /* warn and ignore */

keithwrightbos · 2018-04-13T13:44:01Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+    };
+    Services.urlReplacementsForDoc(ampDoc).expandUrlAsync(
+        errorReportingUrl, macros, whitelist).then(url => {
+      Services.xhrFor(win).fetch(url);


Do we want to send as an XHR? I was thinking we would use an image (see https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/0.1/transport.js#L66)

keithwrightbos · 2018-04-13T13:44:53Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+      ERROR_TYPE: errorType,
+      HREF: win.location.href
+    };
+    Services.urlReplacementsForDoc(ampDoc).expandUrlAsync(


Can't you use expandUrlSync?

keithwrightbos · 2018-04-13T13:46:14Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+ * @param {!Window} win
+ */
+function sendErrorMessage(errorType, errorReportingUrl, win, ampDoc) {
+  if (Math.random() < RTC_ERROR_REPORTING_FREQUENCY) {


Can you make this determined per page instead of per error report? Put the following as a static field:

const ERROR_REPORTING_ENABLED = Math.random() < 0.01;

keithwrightbos · 2018-04-13T18:45:06Z

examples/doubleclick-rtc.amp.html

@@ -17,7 +17,7 @@
 <body>
  <h2> NOTE: Must disable check that isSecureUrl for testing </h2>
  <h2>Doubleclick with RTC</h2>
-    <amp-ad width="320" height="50"
+<!--    <amp-ad width="320" height="50"


keithwrightbos · 2018-04-13T18:45:48Z

extensions/amp-a4a/0.1/real-time-config-manager.js

@@ -32,6 +34,9 @@ const MAX_RTC_CALLOUTS = 5;
 /** @type {number} */
 const MAX_URL_LENGTH = 16384;

+/** @type {boolean} */
+export const ERROR_REPORTING_ENABLED = Math.random() < 0.01;


Why export?

keithwrightbos · 2018-04-13T18:49:23Z

extensions/amp-a4a/0.1/test/test-real-time-config-manager.js

@@ -511,12 +513,14 @@ describes.realWin('real-time-config-manager', {amp: true}, env => {

    // Test various misconfigurations that are missing vendors or urls.
    [{'timeoutMillis': 500}, {'vendors': {}}, {'urls': []},
-      {'vendors': {}, 'urls': []},
-      {'vendors': 'incorrect', 'urls': 'incorrect'}].forEach(rtcConfig => {
+      /*{'vendors': {}, 'urls': []},


remove commented out portion

keithwrightbos · 2018-04-13T18:50:48Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+      dev().warn(TAG, `Insecure RTC errorReportingUrl: ${errorReportingUrl}`);
+      return;
+    }
+    const whitelist = {ERROR_TYPE: true, HREF: true};


We should probably include the timeout macro set by the pub if the errorType is timeout

discussed offline

zhouyx

My biggest concern is that this change adds size to v0.js. ping @choumx and @jridgewell for better ideas.

zhouyx · 2018-04-16T16:56:52Z

src/transport.js

@@ -14,27 +14,27 @@
 * limitations under the License.
 */

-import {Services} from '../../../src/services';
+import {Services} from './services';


ping @jridgewell @choumx
What would be a better approach to share code between two extensions.

zhouyx · 2018-04-16T17:02:03Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+ * @param {!Window} win
+ * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampDoc
+ */
+export function sendErrorMessage(errorType, errorReportingUrl, win, ampDoc) {


<amp-analytics> supports sending user error, that could be another way to support this.
Is error collecting temporary? or is this a feature?

This is a feature of RTC. We wanted a way to do this outside of requiring pubs setup amp-analytics ideally

can we insert analytics like what we do with activeView?

zhouyx · 2018-04-16T21:24:49Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+    };
+    const url = Services.urlReplacementsForDoc(ampDoc).expandUrlSync(
+        errorReportingUrl, macros, whitelist);
+    sendRequest(win, url, {image: true});


If I understand this correct, this will always send a request with image. Then why importing from transport.js, we can simply create new Image() with src equals to url. Let me know if that works. Thanks.

zhouyx

LGTM with one comment

zhouyx · 2018-04-17T19:43:53Z

extensions/amp-a4a/0.1/real-time-config-manager.js

+ * @param {!Window} win
+ * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampDoc
+ */
+export function sendErrorMessage(errorType, errorReportingUrl, win, ampDoc) {


win is not required, can get it from ampdoc.win.
nit: We usually put ampdoc as the first param.

* Add pingback * Change fraction * Allow HREF as macro * Responding to feedback * Respond to feedback * Fix lint * Respond to feedback * Update whitelist * Unmove transport.js * Respond to feedback * Fix mistakes * More responding * Undo class change * Fix tests

googlebot added the cla: yes label Apr 11, 2018

keithwrightbos requested changes Apr 13, 2018

View reviewed changes

bradfrizzell force-pushed the frizz-rtc-pingback branch from 131b2c4 to ff78db5 Compare April 13, 2018 18:11

keithwrightbos approved these changes Apr 13, 2018

View reviewed changes

keithwrightbos requested a review from zhouyx April 13, 2018 18:51

ampproject deleted a comment from keithwrightbos Apr 13, 2018

bradfrizzell mentioned this pull request Apr 16, 2018

Getting lots of travis errors due to changes to the test runner, unclear how to proceed #14636

Closed

zhouyx suggested changes Apr 16, 2018

View reviewed changes

bradfrizzell added 13 commits April 17, 2018 11:54

Add pingback

2c404fb

Change fraction

6ce3be6

Allow HREF as macro

3bb5cc4

Responding to feedback

e00b18a

Respond to feedback

5afd146

Fix lint

433a775

Respond to feedback

5b414f3

Update whitelist

1f6841e

Unmove transport.js

1af4129

Respond to feedback

1a40ef4

Fix mistakes

b0b57bb

More responding

0e0a70f

Undo class change

eaf1286

bradfrizzell force-pushed the frizz-rtc-pingback branch from 6b1c534 to eaf1286 Compare April 17, 2018 15:54

Fix tests

ee2a405

zhouyx approved these changes Apr 17, 2018

View reviewed changes

keithwrightbos merged commit 88eb095 into master Apr 17, 2018

This was referenced Apr 18, 2018

Update RTC Docs to reflect new error pingback capability #14699

Merged

Timeout pixel that fires if the RTC vendor doesn't respond on time #13264

Closed

bradfrizzell deleted the frizz-rtc-pingback branch May 31, 2018 14:45

Send 1% sampled RTC error pings #14563

Send 1% sampled RTC error pings #14563

Uh oh!

Conversation

bradfrizzell commented Apr 11, 2018

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!

zhouyx left a comment

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!

zhouyx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!