For #11961: collect telemetry on all unique unsupported features that… #11969

emalysz · 2020-06-04T18:05:36Z

… trigger fallback error.

This expands upon the telemetry we are collecting that shows the fallback error.

emalysz · 2020-06-04T18:06:48Z

web/app.js

@@ -867,6 +868,15 @@ const PDFViewerApplication = {
      typeof PDFJSDev === "undefined" ||
      PDFJSDev.test("MOZCENTRAL || GENERIC")
    ) {
+      if (featureId && !this._unsupportedFeatureErrors.includes(featureId)) {


null check for featureId because of the changes made in #11953. That would trigger fallback() with a null featureId

Unless I'm mistaken, wouldn't it be a lot simpler/safer to do this kind of validation/tracking of seen featureIds in the new case that you're adding to ChromeActions.reportTelemetry and simply report unconditionally here instead (note how e.g. documentStats is implemented)?

Since that's the central spot where all telemetry results pass through, you'd be thus be guaranteed to only report a specific featureId value once.
By tracking seen featureIds manually in web/app.js, as is done in this patch, it seems much easier for any future changes to accidentally e.g. double-report telemetry data if one forgets to add a this._unsupportedFeatureErrors.push(featureId); line.

timvandermeij · 2020-06-04T22:00:06Z

I think I'm missing something here, but we already have the PDF_VIEWER_FALLBACK_REASON telemetry probe here, so we should already collect the unique unsupported features? Or is the issue that we're currently only reporting the first error even if there are more?

emalysz · 2020-06-04T22:10:28Z

Hey @timvandermeij, sorry if I wasn't clear about this. The PDF_VIEWER_FALLBACK_REASON tells us the reason the fallback bar is shown. However, forms can have multiple errors (https://www.irs.gov/pub/irs-pdf/f1040.pdf will have form, javascript, and font errors). Since only the first error is shown, we wanted to add telemetry to see the total number of errors that trigger the fallback (meaning most are not reflected in the fallback bar). This probe would allow us to record all three of those above errors, not just form.

timvandermeij · 2020-06-04T22:11:41Z

web/app.js

@@ -1235,6 +1244,11 @@ const PDFViewerApplication = {
          return false;
        }
        console.warn("Warning: JavaScript is not supported");
+        this.externalServices.reportTelemetry({


Is there a reason why this cannot be done (unconditionally) in the fallback method? There are more kinds of unsupported features, so why are only these special-cased here?

Yeah I originally thought the same thing, but was hesitant given my work in #11953 (which I think will land before this patch). There, we would trigger the fallback for form and script errors only when the user clicks, and featureId would be null.

So once that lands, I could implement a loop to report the errors within this._delayedFallbackFeatureIds in the fallback method, or the errors could be reported in these two places when we trigger a console.warn. Which would you prefer?

Good point. I'm thinking it's easiest if we can consolidate the telemetry reporting in one place, but we can see how it looks once the other PR is merged.

Unless I'm mistaken, by making these changes in the latest version of the patch it's no longer guaranteed that telemetry reporting happens for the delayed fallback cases.

Consider the case where a document only contains "forms" (and nothing else that'd trigger fallback), and where the user does not interact with the viewer (i.e. no click and/or tab). In that case, the way I'm reading the code, there will not be any PDFViewerApplication.fallback call and thus no telemetry data reported.

Thanks for pointing that out @Snuffleupagus. I opted to create a helper method to avoid reportTelemetry code duplication.

Another option I considered was calling fallback here but passing in an optional bool that would tell us to immediately exit the function following the telemetry collection.

Which solution do you each prefer? @timvandermeij

Another option I considered was calling fallback here but passing in an optional bool that would tell us to immediately exit the function following the telemetry collection.

That honestly sounds worse than the current solution, as far as I'm concerned :-)

(There may be potential for some additional clean-up, since I do think that we're now tracking a bit more state than is probably desirable, but that's perhaps better for a follow-up patch.)

timvandermeij · 2020-06-04T22:14:26Z

Since only the first error is shown, we wanted to add telemetry to see the total number of errors that trigger the fallback (meaning most are not reflected in the fallback bar).

Ah, that was the missing piece for me. Thank you for clarifying that! I was always under the impression that we reported all errors instead of only the first one.

I'm not sure if the existing probe PDF_VIEWER_FALLBACK_REASON is a bit misleading in a sense as I would expect all fallback reasons to be reflected in that, not just one of them. Shouldn't we repurpose that one instead of adding a new one, or are we then worried about skewed data?

web/app.js

timvandermeij · 2020-06-08T21:55:31Z

/botio-linux preview

pdfjsbot · 2020-06-08T21:55:32Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/cd42dcd629e9bac/output.txt

pdfjsbot · 2020-06-08T21:59:02Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/cd42dcd629e9bac/output.txt

Total script time: 3.48 mins

Published

Viewer: http://54.67.70.0:8877/cd42dcd629e9bac/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/cd42dcd629e9bac/es5/web/viewer.html

timvandermeij · 2020-06-09T22:04:13Z

Looks good to me. Can we already land this or should we wait for the upstream change to be done first?

timvandermeij · 2020-06-11T21:43:37Z

/botio-linux preview

pdfjsbot · 2020-06-11T21:43:38Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/2067121fe907c7b/output.txt

pdfjsbot · 2020-06-11T21:47:05Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/2067121fe907c7b/output.txt

Total script time: 3.43 mins

Published

Viewer: http://54.67.70.0:8877/2067121fe907c7b/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/2067121fe907c7b/es5/web/viewer.html

timvandermeij · 2020-06-11T21:49:29Z

web/app.js

+  _recordFallbackErrorTelemetry(featureId) {
+    if (
+      typeof PDFJSDev === "undefined" ||
+      PDFJSDev.test("MOZCENTRAL || GENERIC")


Why GENERIC here as well? As far as I can see reportTelemetry is only implemented and useful in MOZCENTRAL builds.

…es that trigger fallback error. This expands upon the telemetry we are collecting that shows the fallback error.

timvandermeij · 2020-06-12T22:04:11Z

Thank you!

emalysz commented Jun 4, 2020

View reviewed changes

timvandermeij added the viewer label Jun 4, 2020

timvandermeij reviewed Jun 4, 2020

View reviewed changes

Snuffleupagus mentioned this pull request Jun 5, 2020

For #11838: trigger fallback bar after user clicks in pdf #11953

Merged

emalysz force-pushed the 11961-unsupported-feature-telemetry-error branch from 366b639 to 5933ea7 Compare June 5, 2020 22:49

timvandermeij reviewed Jun 6, 2020

View reviewed changes

web/app.js Outdated Show resolved Hide resolved

emalysz force-pushed the 11961-unsupported-feature-telemetry-error branch from 5933ea7 to 6eb41c9 Compare June 8, 2020 15:36

emalysz force-pushed the 11961-unsupported-feature-telemetry-error branch 2 times, most recently from 6b41f15 to 372859b Compare June 11, 2020 17:01

timvandermeij reviewed Jun 11, 2020

View reviewed changes

emalysz force-pushed the 11961-unsupported-feature-telemetry-error branch from 372859b to 9faf233 Compare June 11, 2020 22:04

For mozilla#11961: collect telemetry on all unique unsupported featur…

05fe9c8

…es that trigger fallback error. This expands upon the telemetry we are collecting that shows the fallback error.

emalysz force-pushed the 11961-unsupported-feature-telemetry-error branch from 9faf233 to 05fe9c8 Compare June 11, 2020 22:13

emalysz requested a review from timvandermeij June 12, 2020 19:04

timvandermeij approved these changes Jun 12, 2020

View reviewed changes

timvandermeij merged commit d75a068 into mozilla:master Jun 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

For #11961: collect telemetry on all unique unsupported features that… #11969

For #11961: collect telemetry on all unique unsupported features that… #11969

emalysz commented Jun 4, 2020

emalysz Jun 4, 2020

Snuffleupagus Jun 5, 2020 •

edited

Loading

timvandermeij commented Jun 4, 2020 •

edited

Loading

emalysz commented Jun 4, 2020

timvandermeij Jun 4, 2020

emalysz Jun 4, 2020

timvandermeij Jun 4, 2020

Snuffleupagus Jun 10, 2020

emalysz Jun 11, 2020

Snuffleupagus Jun 11, 2020

timvandermeij commented Jun 4, 2020 •

edited

Loading

timvandermeij commented Jun 8, 2020

pdfjsbot commented Jun 8, 2020

pdfjsbot commented Jun 8, 2020

timvandermeij commented Jun 9, 2020

timvandermeij commented Jun 11, 2020

pdfjsbot commented Jun 11, 2020

pdfjsbot commented Jun 11, 2020

timvandermeij Jun 11, 2020

timvandermeij commented Jun 12, 2020

For #11961: collect telemetry on all unique unsupported features that… #11969

For #11961: collect telemetry on all unique unsupported features that… #11969

Conversation

emalysz commented Jun 4, 2020

Choose a reason for hiding this comment

Snuffleupagus Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

timvandermeij commented Jun 4, 2020 • edited Loading

emalysz commented Jun 4, 2020

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

timvandermeij commented Jun 4, 2020 • edited Loading

timvandermeij commented Jun 8, 2020

pdfjsbot commented Jun 8, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 8, 2020

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Jun 9, 2020

timvandermeij commented Jun 11, 2020

pdfjsbot commented Jun 11, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jun 11, 2020

From: Bot.io (Linux m4)

Success

Published

Choose a reason for hiding this comment

timvandermeij commented Jun 12, 2020

Snuffleupagus Jun 5, 2020 •

edited

Loading

timvandermeij commented Jun 4, 2020 •

edited

Loading

timvandermeij commented Jun 4, 2020 •

edited

Loading