Won't all skins want error reporting abilities? I thought MobileFrontend was supposed to eventually be a generic extension that could support multiple skins. Based on prior discourse, I thought that MobileFrontend kindof sortof had to support at least Vector so we should report when a MobileFrontend + Vector error occurs. Also, most of our dicey JavaScript changes over the next year will be in MobileFrontend, not Minerva. mw.trackSubscribe() will queue global.error failures that occurred prior to invocation but don't you think that subscribing earlier is safer to ensure the invocation is actually run? mobile.init or mobile.startup seem like good possibilities.

Stephen and I discussed MobileFrontend or Minerva - I've updated task description to include this distinction/decision.

We estimated this as a pessimistic 8. This might be a 5 as most of the risk can be deferred to the deploy task, but given some of the team's inexperience with EventLogging stack we settled higher.

Change 455064 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Client side error reporting for Minerva+MobileFrontend

https://gerrit.wikimedia.org/r/455064

Time to get excited! First patch is up and mostly done. I just need to wire up the configuration.

I've defined https://meta.wikimedia.org/wiki/Schema:ClientSideError and I've made a few editorial changes to the names provided in the description.

Given gitMobileFrontend and gitSkin are difficult to obtain, I'm sending wgVersion which should be good enough for production error logging.

uselang

I'm not sure what kind of errors we are looking to obtain here. I'm not aware of any client side errors that would be localised to a certain language. Can you elaborate why this is important?

meta

I'm not sure what was intended here. Does somebody (@Niedzielski ?) want to elaborate on what this is?

mfErrorLoggerEnabled - Whether or not to enable ErrorLogger. When false, ErrorLogger is replaced with a console.log implementation. This config defaults to a sensible value for third-party installations, false.

This doesn't really make sense given default mediawiki behaviour is to console.warn these errors.

In T202026#4528900, @Jdlrobson wrote:

I've defined https://meta.wikimedia.org/wiki/Schema:ClientSideError and I've made a few editorial changes to the names provided in the description.

Any reason why you chose that name over MinervaErrorLog, suggested in the description?

By including the URL as data in the schema, do we run the risk of generating events that are too long for the EventLogging processors to process like we did with the VirtualPageview schema (see T196904)?

ClientSideError

Isn't all EventLogging client-side?

gitMobileFrontend and gitSkin

I think we'll have an easier time injecting these post-Webpack.

uselang

It seems like it would be very useful to easily filter errors by language codes. My recollection was that a similar field in the Android app crash reporter was invaluable for reproduction given the number of wikis we support.

meta

This is an arbitrary map we can use to report more detail as needed, particularly when an error is known to occur but devs are unable to repro. For example, let's say we're consistently failing in WatchstarGateway but we're not sure what the conditions are. We can add meta.watchstarPageIndex and meta.watchstarPageIDs or something like that to better characterize the error.

By including the URL as data in the schema, do we run the risk of generating events that are too long for the EventLogging processors to process

This is hidden deep in the task description but is considered:

Reports that exceed 2000 characters should trim stack and then url properties until within limits. Creative abbreviation or compression may be used to make the fields more applicable and concise.

👍

Note that uselang does not have much to do with the language of the error, unless you are throwing error messages you get from the API. The error message language is determined by navigator.language, probably.

Per language, the event logging capsule contains domain and project so default language is available. Since it's not possible to change UI language I dont think this is needed.

Per name: by using something generic I thought it would be useful since we could reserve the right to add it into other places e.g. Popups
There is nothing Minerva specific inside it.

Thanks for additional information on meta. I'll add it to schema but sounds like don't need to make use of it right now.

In terms of length I think the stacktrace is going to be more problematic here. Trimming the uri is problematic here as the hash fragment is important information (more so than the rest). Personally I would not want to trim this until it's clear what the error rate is - remember these errors are not on every page view like VirtualPageViews.

Change 455064 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Client side error reporting for Minerva+MobileFrontend

https://gerrit.wikimedia.org/r/455064

Skipping design and QA as the code should be inert.

We've merged a first version of the error handling. Next steps would be to enable it to a tiny % of users. No rush on that front, just yet. We should also explore putting this on the beta cluster which traffic is smaller. I'm not sure if that gets piped to Grafana however..

To whomever picks this up, additional feedback here: https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/455064.

Change 458588 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Hygiene: Move constants and explicit dependencies to top of errorLogging

https://gerrit.wikimedia.org/r/458588

Change 458589 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Hygiene: Consistent error naming

https://gerrit.wikimedia.org/r/458589

Change 458590 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Hygiene: Load errorLogger earlier in stack

https://gerrit.wikimedia.org/r/458590

Change 458591 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Log a character limited stack trace in error reports

https://gerrit.wikimedia.org/r/458591

Change 458590 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Load errorLogger earlier in stack

https://gerrit.wikimedia.org/r/458590

Change 458589 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: Consistent error naming

https://gerrit.wikimedia.org/r/458589

Change 458588 abandoned by Jdlrobson:
Hygiene: Move constants and explicit dependencies to top of errorLogging

Reason:
not working on this (toomanythings)

https://gerrit.wikimedia.org/r/458588

Change 458591 abandoned by Jdlrobson:
Log a character limited stack trace in error reports

Reason:
Let's continue this conversation in https://phabricator.wikimedia.org/T203812

https://gerrit.wikimedia.org/r/458591

I suggest we create tasks for any further follow ups, given we still need to check the viability of error reporting and whether it's useful to use EventLogging.

I've setup T203812 to track the issue with stack traces.
I'd like us to get a sense of the number of errors we are getting in production before throwing more energy into this.

Failures are to be reported in Grafana and usable for investigation via SSH. Set up a task to make that happen.

I think this AC is incorrect and that reports generated should immediately be visible in Grafana without a new task. However, I'm not seeing them! Please see notes below.

I've tested this on readers-web-master and copy and pasted the EventLogging URL generated into a curl command pointed at the beta cluster.

That is, I changed this URL:

https://readers-web-master.wmflabs.org/event.gif?%7B%22event%22%3A%7B%22userUrl%22%3A%22https%3A%2F%2Freaders-web-master.wmflabs.org%2Fwiki%2FPharmacovigilance%22%2C%22errorUrl%22%3A%22%22%2C%22errorMessage%22%3A%22Script%20error.%22%2C%22errorStackTrace%22%3A%22%22%2C%22errorLineNumber%22%3A0%2C%22errorColumnNumber%22%3A0%2C%22sessionToken%22%3A%228bdfd3d953188fe33881%22%2C%22skin%22%3A%22minerva%22%2C%22wgVersion%22%3A%221.32.0-alpha%22%2C%22mobileMode%22%3A%22beta%22%2C%22isAnon%22%3Afalse%2C%22revision%22%3A0%7D%2C%22revision%22%3A18340282%2C%22schema%22%3A%22WebClientError%22%2C%22webHost%22%3A%22readers-web-master.wmflabs.org%22%2C%22wiki%22%3A%22wiki%22%7D;

To the following command:

curl -A "WikipediaApp/22.0.22-alpha-2017-11-01 (Android 8.0.0; Phone) Alpha Channel" 'https://en.wikipedia.beta.wmflabs.org/beacon/event.gif?%7B%22event%22%3A%7B%22userUrl%22%3A%22https%3A%2F%2Freaders-web-master.wmflabs.org%2Fwiki%2FPharmacovigilance%22%2C%22errorUrl%22%3A%22%22%2C%22errorMessage%22%3A%22Script%20error.%22%2C%22errorStackTrace%22%3A%22%22%2C%22errorLineNumber%22%3A0%2C%22errorColumnNumber%22%3A0%2C%22sessionToken%22%3A%228bdfd3d953188fe33881%22%2C%22skin%22%3A%22minerva%22%2C%22wgVersion%22%3A%221.32.0-alpha%22%2C%22mobileMode%22%3A%22beta%22%2C%22isAnon%22%3Afalse%2C%22revision%22%3A0%7D%2C%22revision%22%3A18340282%2C%22schema%22%3A%22WebClientError%22%2C%22webHost%22%3A%22readers-web-master.wmflabs.org%22%2C%22wiki%22%3A%22wiki%22%7D;'

Which was output on deployment-eventlogging05.eqiad.wmflabs /srv/log/eventlogging/all-events.log:

{
  "dt": "2018-09-10T20:04:06Z",
  "event": {
    "errorColumnNumber": 0,
    "errorLineNumber": 0,
    "errorMessage": "Script error.",
    "errorStackTrace": "",
    "errorUrl": "",
    "isAnon": false,
    "mobileMode": "beta",
    "revision": 0,
    "sessionToken": "8bdfd3d953188fe33881",
    "skin": "minerva",
    "userUrl": "https://readers-web-master.wmflabs.org/wiki/Pharmacovigilance",
    "wgVersion": "1.32.0-alpha"
  },
  "ip": "73.181.4.128",
  "recvFrom": "deployment-cache-text04.deployment-prep.eqiad.wmflabs",
  "revision": 18340282,
  "schema": "WebClientError",
  "seqId": 45215402,
  "userAgent": {
    "browser_family": "Android",
    "browser_major": "8",
    "browser_minor": "0",
    "device_family": "Generic Smartphone",
    "is_bot": false,
    "is_mediawiki": false,
    "os_family": "Android",
    "os_major": "8",
    "os_minor": "0",
    "wmf_app_version": "22.0.22-alpha-2017-11-01"
  },
  "uuid": "3e074052bb99509f875f5b92a4c4cf0f",
  "webHost": "readers-web-master.wmflabs.org",
  "wiki": "wiki"
}

However, the event was not shown on Grafana:

Chores will need to be updated to include checking the new Grafana reports.

I didn't see an updates in the chore wheel template and chore list page.

A task is created to evaluate the future of JavaScript error reporting at the end of the MobileFrontend investment project: do we keep our hacky solution, graduate to Sentry, delete the hack, or something else?

I'm sorry if I've missed this task but I don't believe one has been created yet

Lastly, infinite looping and MobileFrontend logging concerns identified in code review were not addressed and seem like blockers to me.

That’s production Grafana :-)

You’ll want to check want to check Beta Cluster’s own Graphite/Grafana.

You’ll want to check want to check Beta Cluster’s own Graphite/Grafana.

Thanks @Krinkle.

I tried creating a dashboard on https://grafana-labs.wikimedia.org/dashboard/db/niedzielski-eventlogging-schema?var-schema=VirtualPageView with help from @Ottomata but ran out of time. Getting alerts on beta before it hits prod would be a wonderful improvement.

I retested using prod EventLogging and dropped the .gif from the URL:

curl -A "WikipediaApp/22.0.22-alpha-2017-11-01 (Android 8.0.0; Phone) Alpha Channel" 'https://en.wikipedia.beta.wmflabs.org/beacon/event?%7B%22event%22%3A%7B%22userUrl%22%3A%22https%3A%2F%2Freaders-web-master.wmflabs.org%2Fwiki%2FPharmacovigilance%22%2C%22errorUrl%22%3A%22%22%2C%22errorMessage%22%3A%22Script%20error.%22%2C%22errorStackTrace%22%3A%22%22%2C%22errorLineNumber%22%3A0%2C%22errorColumnNumber%22%3A0%2C%22sessionToken%22%3A%228bdfd3d953188fe33881%22%2C%22skin%22%3A%22minerva%22%2C%22wgVersion%22%3A%221.32.0-alpha%22%2C%22mobileMode%22%3A%22beta%22%2C%22isAnon%22%3Afalse%2C%22revision%22%3A0%7D%2C%22revision%22%3A18340282%2C%22schema%22%3A%22WebClientError%22%2C%22webHost%22%3A%22readers-web-master.wmflabs.org%22%2C%22wiki%22%3A%22wiki%22%7D;'

This worked:

\o/

Lastly, infinite looping and MobileFrontend logging concerns identified in code review were not addressed and seem like blockers to me.

@Jdlrobson asked for more clarity on these points in standup:

• I'm guessing that calling M.require() within the error handler in the case that the file it requires has an error does not create an infinite loop but I would like that to be verified.
• I'm concerned that most of our JavaScript changes over the next year will occur in MobileFrontend but may not be reported by this mechanism due to load order. Correct me if I'm wrong but if there's an error that occurs when loading mobile.startup, we'll never hear about it through this new report mechanism.

I'm guessing that calling M.require() within the error handler in the case that the file it requires has an error does not create an infinite loop but I would like that to be verified.

Correct. No infinite loop. Throwing an error in the error handler doesn't invoke the hook.

I'm concerned that most of our JavaScript changes over the next year will occur in MobileFrontend but may not be reported by this mechanism due to load order. Correct me if I'm wrong but if there's an error that occurs when loading mobile.startup, we'll never hear about it through this new report mechanism.

It depend where it occurs. When loading yes. Issues inside the top level closure could be missed yes. mobile.init is also too late. The only way to fix this would be to insert the handler at the front of mobile.startup, which would also load error reporting for anything else that happens to use it e.g. gadgets. It would also mean the loading of mobile.startup has side effects.

I'd argue those kind of errors are also going to be very visible to us and reproducible as if something fails in mobile.startup load the entire mobile site JS fails to load. If this indeed happens this could be a problem as it would be spammy for all UA's impacted.

We'd still catch issues inside the code - for instance if new Overlay() throws an error we'd hear about it

We can revisit this later, but I'm fine missing those kind of issues from the start until we feel more confident with the sampling rate.

Correct. No infinite loop. Throwing an error in the error handler doesn't invoke the hook.

Thanks for verifying!

The only way to fix this would be to insert the handler at the front of mobile.startup, which would also load error reporting for anything else that happens to use it e.g. gadgets. It would also mean the loading of mobile.startup has side effects.

Broken gadgets can cause problems at any time and will always need to be considered when viewing trends. Covering our eyes while gadgets _and_ MobileFrontend are loading isn't a solution in my opinion.

I'd argue those kind of errors are also going to be very visible to us and reproducible as if something fails in mobile.startup load the entire mobile site JS fails to load. If this indeed happens this could be a problem as it would be spammy for all UA's impacted.

I agree that that's generally true for mobile.startup code but what about less frequented modules of which there is a significant portion of code? For example, I don't know when the last time I checked nearby was and loading errors there would only be reported if we move where this handler is executed. I would rather file tickets to fix gadgets or filter out entire misbehaving wikis than miss out on legit MobileFrontend regressions we introduce when we actually introduce them.

Change 464373 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[operations/mediawiki-config@master] Log client errors on beta cluster at 100%

https://gerrit.wikimedia.org/r/464373

Change 464373 merged by jenkins-bot:
[operations/mediawiki-config@master] Beta: Log client errors on beta cluster at 100%

https://gerrit.wikimedia.org/r/464373

I've verified that errors are reported in grafana.
You can trigger an error using the following code:

window.setTimeout(function () {
	mw.stub.call();
}, 3000 );
$.ajax({url: '/' }).then( function() {
	mw.stub.call();
})

but not

mw.loader.using( 'mobile.startup' ).then(function() {
	mw.stub.call();
})

@Krinkle is there any reason errors inside a mw.loader.using callback are not trigger global.error or resourceloader.exception ?

Some replies to earlier discussions:

Broken gadgets can cause problems at any time and will always need to be considered when viewing trends. Covering our eyes while gadgets _and_ MobileFrontend are loading isn't a solution in my opinion.

Sorry, I have been a little confusing here. Gadgets are no different from our code, so gadgets that run after our code (which is always the case from my understanding) and will lead to errors being logged in our system. However, what I was concerned about here was desktop gadgets using our code and triggering errors outside the context of our own ecosystem. The error logger right now is enabled only in Minerva, meaning it cannot be triggered inside Vector or any other skin. I think this is a good thing as it keeps the environment predictable.

don't know when the last time I checked nearby was and loading errors there would only be reported if we move where this handler is executed.

That's not true. Nearby Minerva errors would be logged. I'm not sure we want to track errors inside Vector Minerva do we (since we don't officially support it)?

. I would rather file tickets to fix gadgets or filter out entire misbehaving wikis than miss out on legit MobileFrontend regressions we introduce when we actually introduce them.

You can't file tickets against gadgets. If a gadget is throwing an error the only way is to hope that gadget is still being maintained and/or edit the gadget itself :/

I added a breakpoint to the search bar click listener. When it paused, I caused a TypeError by changing a variable to null. The exception was thrown from:

load.php?debug=false&lang=en&modules=ext.centralNotice.choiceData%2Cdisplay%2CgeoIP%2CimpressionEventsSampleRate%2CkvStore%2CkvStoreMaintenance%2CstartUp|ext.centralauth.centralautologin.clearcookie|ext.eventLogging%2CnavigationTiming%2CwikimediaEvents|ext.eventLogging.debug%2Csubscriber|ext.quicksurveys.init%2Clib|ext.visualEditor.targetLoader%2Ctrack%2Cve|ext.wikimediaEvents.loggedin|jquery%2Coojs%2Coojs-router|jquery.accessKeyLabel%2Cclient%2Ccookie%2CtextSelection%2Cthrottle-debounce|mediawiki.RegExp%2CString%2CTitle%2CUri%2Capi%2Cbase%2Ccldr%2Ccookie%2Cexperiments%2CjqueryMsg%2Clanguage%2Cnotify%2Crouter%2Cstorage%2Ctemplate%2Cuser%2Cutil%2Cviewport|mediawiki.legacy.wikibits|mediawiki.libs.pluralruleparser|mediawiki.page.startup|mediawiki.template.hogan%2Cregexp|mediawiki.ui.anchor%2Cinput|mobile.ajax.styles|mobile.editor.api|mobile.init%2Creferences%2Csearch%2Csite%2Cstartup%2Ctoggle%2Cwatchstar|mobile.pagelist.scripts%2Cstyles|mobile.pagesummary.styles|mobile.references.gateway%2Cimages|mobile.search.api%2Cimages%2Cutil|mobile.site.styles|mobile.startup.images|mobile.startup.images.variants|schema.QuickSurveyInitiation|skins.minerva.editor%2CmainMenu%2Cnotifications%2Cscripts%2Ctoggling|skins.minerva.icons.images.scripts%2Cvariants|skins.minerva.icons.images.scripts.misc|skins.minerva.icons.page.issues.default.color|skins.minerva.icons.page.issues.medium.color|skins.minerva.icons.page.issues.uncolored|skins.minerva.mainMenu.icons%2Cstyles|skins.minerva.notifications.badge|skins.minerva.scripts.top|user.defaults&skin=minerva&version=1hi88f9:formatted:9823

The new handler properly intercepts the error but is unable to notify anyone about it: "Url exceeds maximum length". We should instead the report the error with a truncated URL in this case so that we see a spike of errors in EventLogging when they're occurring whether or not we're able to report exactly where they occur.

I tried the again by adding a breakpoint to doPrintBeforeTimeout which is invoked by clicking the download button. When the breakpoint hit, I nulled out this.showSpinner and saw the browser 204 the event:

https://en.wikipedia.beta.wmflabs.org/beacon/event?%7B%22event%22%3A%7B%22userUrl%22%3A%22https%3A%2F%2Fen.m.wikipedia.beta.wmflabs.org%2Fwiki%2FBarack_Obama%22%2C%22errorUrl%22%3A%22%22%2C%22errorMessage%22%3A%22Uncaught%20TypeError%3A%20this.showSpinner%20is%20not%20a%20function%22%2C%22errorStackTrace%22%3A%22%22%2C%22errorLineNumber%22%3A189%2C%22errorColumnNumber%22%3A22%2C%22sessionToken%22%3A%220466bfe4645502de05dc%22%2C%22skin%22%3A%22minerva%22%2C%22wgVersion%22%3A%221.32.0-alpha%22%2C%22mobileMode%22%3A%22stable%22%2C%22isAnon%22%3Afalse%2C%22revision%22%3A377511%7D%2C%22revision%22%3A18340282%2C%22schema%22%3A%22WebClientError%22%2C%22webHost%22%3A%22en.m.wikipedia.beta.wmflabs.org%22%2C%22wiki%22%3A%22enwiki%22%7D;

I see the error logged on the BC dashboard and my hacky dashboard.

I'd like to continue the discussion of when we should start listening for errors so I've opened T206248.

The new handler properly intercepts the error but is unable to notify anyone about it: "Url exceeds maximum length". We should instead the report the error with a truncated URL in this case so that we see a spike of errors in EventLogging when they're occurring whether or not we're able to report exactly where they occur.

We talked a lot about truncating things during code review and bike shedded a lot so we removed most of it. Which field is the issue here that is leading to the long URI ? Is it the errorMessage itself?

Good question. I was thinking maybe the URL itself? Even a simple truncation of the URL field seems worthwhile to me.

Change 464640 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Simplify WebClientError code

https://gerrit.wikimedia.org/r/464640

mw.loader.using( 'mobile.startup' ).then(function() {
	mw.stub.call();
})

So investigating this some more the problem here, is that the callback is outside of the scope of mw.loader and you'd need to capture it like so:

mw.loader.using( 'mobile.startup' ).then(function() {
	mw.stub.call();
}).catch( function() {
} );

A reminder that we'll need to add explicit catch handlers if we want to track errors in any code that uses this pattern.

Good question. I was thinking maybe the URL itself? Even a simple truncation of the URL field seems worthwhile to me.

Have setup T206257 to investigate (good news is we still capture these via the EventError schema or can access these via logs so I don't think they are completely lost).

https://gerrit.wikimedia.org/r/464640 provides a simplified version of this code but I've setup T206258 to see that progress.

I think we can safely close out this task now.