We observed in Bug 1006947 that sometimes we catch and log home provider database errors, rather than crashing. Rather than change that behavior (with naturally unknown consequences for home panel users -- maybe this happens a lot!), I suggest we add a telemetry probe. This will allow us to gauge the success of a fix for Bug 1006947 without relying on a very low crash rate.

Well, this builds... margaret, thoughts?

Comment on attachment 8431059 [details] [diff] [review] Telemetry probe for home provider database errors. Review of attachment 8431059 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable to me. Should we add something to indicate which SQLiteBridgeContentProvider implementation this actually stemmed from? E.g. HomeProvider vs. PasswordsProvider. ::: mobile/android/base/db/SQLiteBridgeContentProvider.java @@ +399,5 @@ > } > > + private static String getHistogram(SQLiteBridgeException e, String op) { > + if (ERROR_MESSAGE_DATABASE_IS_LOCKED.equals(e.getMessage())) { > + return HISTOGRAM_SQLITEBRIDGE_PROVIDER_PREFIX + "LOCKED_" + op.toUpperCase(); I think it would be better to make these op strings constants, since they correspond to the histogram name. In fact, instead of making reportError take a string as the second parameter, it could just take the histogram constant we want to use.

Thanks for the feedback! > Looks reasonable to me. Should we add something to indicate which > SQLiteBridgeContentProvider implementation this actually stemmed from? E.g. > HomeProvider vs. PasswordsProvider. I was thinking about that, but I wanted to avoid landing too many probes. We could (ab)use UI telemetry instead, or I can see if telemetry has better constructs... > I think it would be better to make these op strings constants, since they > correspond to the histogram name. In fact, instead of making reportError > take a string as the second parameter, it could just take the histogram > constant we want to use. I wanted to avoid the code reporting the error having to determine what kind of exception this was (e.g., this was an insert, and it's a DB locked exception, so I want SQLITEBRIDGE_PROVIDER_LOCKED_INSERT). I'll try to figure out a neater way to slice this.

Switched to using an enumerated histogram, one per provider. Much neater.

Tested by recording fake exceptions; added to telemetry successfully. Let me know if you want me to try to figure out an automated test for this. mfinkle, feel free to steal this one.

I'd like to fling this into Aurora ASAP, so we can catch a few days of data to validate our fix in Bug 1006947, which hit fx-team today. I'd love to get it into Beta, but I'm pretty sure that's out of the question with a week to go.

Comment on attachment 8432034 [details] [diff] [review] Telemetry probe for home provider database errors. v3 Review of attachment 8432034 [details] [diff] [review]: ----------------------------------------------------------------- I think this version is better, thanks.

Thanks, Margaret! https://hg.mozilla.org/integration/fx-team/rev/0b5b627caf3a needinfo on me to assess for uplift.

Comment on attachment 8432034 [details] [diff] [review] Telemetry probe for home provider database errors. v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): None; telemetry probe addition to evaluate a fix. User impact if declined: We won't know if we fixed an error. We'd like to uplift this a little in advance so that we can get before/after evaluation. Testing completed (on m-c, etc.): Baked. Tested locally with artificial failures. Risk to taking this patch (and alternatives if risky): Slim. Just adds and uses new telemetry histograms. String or IDL/UUID changes made by this patch: None.