We take a lot of time to clear plugins in some cases, but it's unclear if the main culprit is Flash and whether the problem is loading it, or it's just slow clearing data. We can add probes for that.

Review commit: https://reviewboard.mozilla.org/r/36837/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/36837/

I must recall to add bug_numbers to the new probes, since they were made mandatory 5 minutes ago!

Comment on attachment 8724051 [details] MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36837/diff/1-2/

Comment on attachment 8724051 [details] MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric https://reviewboard.mozilla.org/r/36837/#review33429 ::: browser/base/content/sanitize.js:294 (Diff revision 1) > + let perPluginTelemetryProbe = (tag) => { This function is called a single time. Moving this out of line makes the code a bit harder to read. ::: browser/base/content/sanitize.js:296 (Diff revision 1) > + return `FX_SANITIZE_${tag.loaded ? "LOADED" : "UNLOADED"}_FLASH`; I'd prefer if you did ```js return tag.loaded ? "FX_SANITIZER_LOADED_FLASH" : "FX_SANITIZER_UNLOADED_FLASH"; ``` It makes it easier to both spot typos and search in the code. ::: browser/base/content/sanitize.js:308 (Diff revision 1) > + let probe = perPluginTelemetryProbe(tag); Why don't we make this a keyed histogram, with key ```js `${tag.name}: ${tag.loaded}` ```? This will help us pinpoint offenders more easily. Since the Telemetry Ping already gives us the list of plugins (I believe), this doesn't cause any privacy risk. ::: browser/base/content/sanitize.js:323 (Diff revision 1) > + TelemetryStopwatch.finish(probe, refObj); If you just want to know the time spent clearing site data by a plugin (rather than the time spent waiting for the event loop), you should rather put the calls to `TelemetryStopwatch` inside the calls to `new Promise`.

https://reviewboard.mozilla.org/r/36837/#review33429 > Why don't we make this a keyed histogram, with key ```js > `${tag.name}: ${tag.loaded}` > ```? This will help us pinpoint offenders more easily. > > Since the Telemetry Ping already gives us the list of plugins (I believe), this doesn't cause any privacy risk. I don't want to complicate the data more than we need. It's pretty clear Flash is the thing we care most, also because we are going to deprecate all NPAPI plugins bug Flash by end of the year. Plus it's likely Flash is one of the very few plugins supporting clearing data, cause IIRC it was added to NPAPI exactly for Flash. Plus Flash is the only plugin that we will instantiate on sanitize if it's not yet loaded (single item whitelist). So, I don't think it's worth measuring anything outside of flash. > If you just want to know the time spent clearing site data by a plugin (rather than the time spent waiting for the event loop), you should rather put the calls to `TelemetryStopwatch` inside the calls to `new Promise`. What affects the user is the whole thing, and that's what we want to measure. Moreover with lots of data having some skewed results due to a busy events loop doesn't really matter. Thus I think the code complication to move this inside the promise is not worth it, cause then I should change all the callbacks, or I'd just measure the cost of the call.

https://reviewboard.mozilla.org/r/36837/#review33429 > I don't want to complicate the data more than we need. It's pretty clear Flash is the thing we care most, also because we are going to deprecate all NPAPI plugins bug Flash by end of the year. Plus it's likely Flash is one of the very few plugins supporting clearing data, cause IIRC it was added to NPAPI exactly for Flash. > Plus Flash is the only plugin that we will instantiate on sanitize if it's not yet loaded (single item whitelist). > So, I don't think it's worth measuring anything outside of flash. Ok, fair enough. > What affects the user is the whole thing, and that's what we want to measure. Moreover with lots of data having some skewed results due to a busy events loop doesn't really matter. > Thus I think the code complication to move this inside the promise is not worth it, cause then I should change all the callbacks, or I'd just measure the cost of the call. Ok, fair enough.

Comment on attachment 8724051 [details] MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric https://reviewboard.mozilla.org/r/36837/#review33453 In that case, ship it with minor changes.

Comment on attachment 8724051 [details] MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36837/diff/2-3/

Should we uplift this to 46?

we could, but it could also ride the trains. honestly there isn't much we can do in Aurora/Beta for plugins without taking risks.

Hi Marco, I am implementing a function similar to what promiseClearPluginCookies [1] does for the browsingData API. Should I include telemetry probes in my code as well, in the same locations, and if so, should I use the same probe names, so our data can be added to your data, or create new probe names of our own? [1] http://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#314

(In reply to Bob Silverberg [:bsilverberg] from comment #13) > Hi Marco, I am implementing a function similar to what > promiseClearPluginCookies [1] does for the browsingData API. Should I > include telemetry probes in my code as well, in the same locations, and if > so, should I use the same probe names, so our data can be added to your > data, or create new probe names of our own? I think you should create a FX_SANITIZE_YOURDATANAME probe, for coherency, if your code ends up in the sanitizer