Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1094873002:
Extensions: Switch to new permission message system, part V (Closed)
|
Created:
5 years, 8 months ago by Marc Treib Modified:
5 years, 7 months ago CC:
chromium-reviews, dbeam+watch-ntp_chromium.org, extensions-reviews_chromium.org, asvitkine+watch_chromium.org, estade+watch_chromium.org, chromium-apps-reviews_chromium.org, pedrosimonetti+watch_chromium.org, benwells Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: Switch to new permission message system, part V
Add new UMA histograms for APIPermission::ID (which will eventually replace PermissionMessage::ID)
TBRing a mechanical change in app_launcher_handler.cc
TBR=estade
BUG=398257
Committed: https://crrev.com/2e0517f9e7eaf7901a00d8e475c4cf37c534f9e9
Cr-Commit-Position: refs/heads/master@{#328874}
Patch Set 1 #
Total comments: 17
Patch Set 2 : review (also rebase) #Patch Set 3 : presubmit #
Total comments: 8
Patch Set 4 : kalman review #
Total comments: 15
Patch Set 5 : mpearson review #
Total comments: 2
Patch Set 6 : mpearson review2 #
Total comments: 2
Patch Set 7 : mpearson review3 #Patch Set 8 : rebase #
Total comments: 1
Messages
Total messages: 37 (6 generated)
treib@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Next part, with an open question. Please comment! https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:47: kNone, Not quite sure about this: The existing histograms record a special "none" value when an extension requires no permissions, or one value *per permission* otherwise. So e.g. we can't just add all the numbers to get the total number of installations. Maybe it'd be better to not have a "none" entry here, and instead make separate bool histograms ("NeedsPermissions")? https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:221: // TODO(sashab): Move these in-line with the other permission IDs. We'd have to do this reordering right now, before making the new histograms. But I think we shouldn't bother, since we won't be able to keep the list sorted alphabetically anyway. https://codereview.chromium.org/1094873002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1094873002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:50835: - <int value="68" label="kDocumentScan"/> Turns out histograms.xml was out of sync with the actual enum...
Ping!
Just one question (why we record 2 and 3) https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1015: std::string histogram_name_base = Is there a reason to record both 2 and 3? (It doesn't look like we bother recording the original anymore...) https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1016: std::string("Extensions.Permissions_") + histogram; nit: + string concatenation is pretty slow - let's use a StringPrintf here. std::string legacy_histogram_name = base::StringPrintf("Extensions.Permissions_%s2", histogram); std::string histogram_name = base::StringPrintf("Extensions.Permissions_%s3", histogram); https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:47: kNone, On 2015/04/17 13:09:33, Marc Treib wrote: > Not quite sure about this: The existing histograms record a special "none" value > when an extension requires no permissions, or one value *per permission* > otherwise. So e.g. we can't just add all the numbers to get the total number of > installations. Maybe it'd be better to not have a "none" entry here, and instead > make separate bool histograms ("NeedsPermissions")? I agree - it's weird to have "None" as a unique permission type. I like the idea of a boolean histogram. We should maintain the status quo for the existing/legacy one, though. https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:221: // TODO(sashab): Move these in-line with the other permission IDs. On 2015/04/17 13:09:33, Marc Treib wrote: > We'd have to do this reordering right now, before making the new histograms. But > I think we shouldn't bother, since we won't be able to keep the list sorted > alphabetically anyway. Yeah, there's no way we can keep them in order - but I don't know if we should keep this section here at all. Otherwise, we'll be at a stage of enum { <alphabetical> <pseudo-random-section-of-invalid-permissions> <everything else> } Might be best just to inline it now so we at least get enum { <alphabetical> <everything else> }
https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1015: std::string histogram_name_base = On 2015/04/27 17:58:03, Devlin wrote: > Is there a reason to record both 2 and 3? (It doesn't look like we bother > recording the original anymore...) I just didn't want to break the existing ones until I'm sure the new ones fulfill everyone's needs. After having both for a while (say, a week or two on Dev at least), I'll remove the old stuff. https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1016: std::string("Extensions.Permissions_") + histogram; On 2015/04/27 17:58:03, Devlin wrote: > nit: + string concatenation is pretty slow - let's use a StringPrintf here. > std::string legacy_histogram_name = > base::StringPrintf("Extensions.Permissions_%s2", histogram); > std::string histogram_name = base::StringPrintf("Extensions.Permissions_%s3", > histogram); Okay, done. Is "+"-style concatenation really that bad, though?! https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:47: kNone, On 2015/04/27 17:58:03, Devlin wrote: > On 2015/04/17 13:09:33, Marc Treib wrote: > > Not quite sure about this: The existing histograms record a special "none" > value > > when an extension requires no permissions, or one value *per permission* > > otherwise. So e.g. we can't just add all the numbers to get the total number > of > > installations. Maybe it'd be better to not have a "none" entry here, and > instead > > make separate bool histograms ("NeedsPermissions")? > > I agree - it's weird to have "None" as a unique permission type. I like the > idea of a boolean histogram. We should maintain the status quo for the > existing/legacy one, though. I've removed the "None" type for the new histograms, and instead added new boolean "HasPermissions" histograms. https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:221: // TODO(sashab): Move these in-line with the other permission IDs. On 2015/04/27 17:58:03, Devlin wrote: > On 2015/04/17 13:09:33, Marc Treib wrote: > > We'd have to do this reordering right now, before making the new histograms. > But > > I think we shouldn't bother, since we won't be able to keep the list sorted > > alphabetically anyway. > > Yeah, there's no way we can keep them in order - but I don't know if we should > keep this section here at all. Otherwise, we'll be at a stage of > enum { > <alphabetical> > <pseudo-random-section-of-invalid-permissions> > <everything else> > } > > Might be best just to inline it now so we at least get > enum { > <alphabetical> > <everything else> > } I'm a bit concerned that when a developer sees an alphabetical list, they'll be tempted to insert their new entry in the "proper" place in the middle. So maybe a random_shuffle would be better :D
https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1015: std::string histogram_name_base = On 2015/04/28 12:31:32, Marc Treib wrote: > On 2015/04/27 17:58:03, Devlin wrote: > > Is there a reason to record both 2 and 3? (It doesn't look like we bother > > recording the original anymore...) > > I just didn't want to break the existing ones until I'm sure the new ones > fulfill everyone's needs. After having both for a while (say, a week or two on > Dev at least), I'll remove the old stuff. Yeah, I understand the motivation, but I'm not sure it's right to "pollute" the old, if we have our concerns at all. The alternative it to pass in a bool for whether or not to record old metrics, and only record if we're not using the new system. I'm fine either way, though - just making sure you've thought of it. Use your best judgment. :) https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1016: std::string("Extensions.Permissions_") + histogram; On 2015/04/28 12:31:32, Marc Treib wrote: > On 2015/04/27 17:58:03, Devlin wrote: > > nit: + string concatenation is pretty slow - let's use a StringPrintf here. > > std::string legacy_histogram_name = > > base::StringPrintf("Extensions.Permissions_%s2", histogram); > > std::string histogram_name = base::StringPrintf("Extensions.Permissions_%s3", > > histogram); > > Okay, done. Is "+"-style concatenation really that bad, though?! Depends on whom you ask. ;) It's tremendously worse than StringPrintf for > 1 concatenations (here, you have two - the root + the histogram name + the number suffix), but it's also not like this is performance-critical code. But, a good habit, nonetheless, and it doesn't (IMO) hurt readability at all. https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:221: // TODO(sashab): Move these in-line with the other permission IDs. On 2015/04/28 12:31:32, Marc Treib wrote: > On 2015/04/27 17:58:03, Devlin wrote: > > On 2015/04/17 13:09:33, Marc Treib wrote: > > > We'd have to do this reordering right now, before making the new histograms. > > But > > > I think we shouldn't bother, since we won't be able to keep the list sorted > > > alphabetically anyway. > > > > Yeah, there's no way we can keep them in order - but I don't know if we should > > keep this section here at all. Otherwise, we'll be at a stage of > > enum { > > <alphabetical> > > <pseudo-random-section-of-invalid-permissions> > > <everything else> > > } > > > > Might be best just to inline it now so we at least get > > enum { > > <alphabetical> > > <everything else> > > } > > I'm a bit concerned that when a developer sees an alphabetical list, they'll be > tempted to insert their new entry in the "proper" place in the middle. So maybe > a random_shuffle would be better :D I was actually toying with the same idea... and it might not be bad. But we shouldn't leave this section in there; otherwise it forces new entries under this weird section of "not valid permissions". So, we should take out the section (or, at bare minimum, the comment), and if you leave them in alphabetical order, maybe put a comment at the end saying "Initial batch as of <date>; new entries below." Also, I know we have presubmits for things like ExtensionFunction histograms - might be worth looking into if we can just give it another source file/enum to protect?
https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1015: std::string histogram_name_base = On 2015/04/28 16:16:36, Devlin wrote: > On 2015/04/28 12:31:32, Marc Treib wrote: > > On 2015/04/27 17:58:03, Devlin wrote: > > > Is there a reason to record both 2 and 3? (It doesn't look like we bother > > > recording the original anymore...) > > > > I just didn't want to break the existing ones until I'm sure the new ones > > fulfill everyone's needs. After having both for a while (say, a week or two on > > Dev at least), I'll remove the old stuff. > > Yeah, I understand the motivation, but I'm not sure it's right to "pollute" the > old, if we have our concerns at all. The alternative it to pass in a bool for > whether or not to record old metrics, and only record if we're not using the new > system. > > I'm fine either way, though - just making sure you've thought of it. Use your > best judgment. :) What do you mean by "pollute"? The old histograms shouldn't change at all with this CL. https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/1094873002/diff/1/extensions/common/permissio... extensions/common/permissions/api_permission.h:221: // TODO(sashab): Move these in-line with the other permission IDs. On 2015/04/28 16:16:36, Devlin wrote: > On 2015/04/28 12:31:32, Marc Treib wrote: > > On 2015/04/27 17:58:03, Devlin wrote: > > > On 2015/04/17 13:09:33, Marc Treib wrote: > > > > We'd have to do this reordering right now, before making the new > histograms. > > > But > > > > I think we shouldn't bother, since we won't be able to keep the list > sorted > > > > alphabetically anyway. > > > > > > Yeah, there's no way we can keep them in order - but I don't know if we > should > > > keep this section here at all. Otherwise, we'll be at a stage of > > > enum { > > > <alphabetical> > > > <pseudo-random-section-of-invalid-permissions> > > > <everything else> > > > } > > > > > > Might be best just to inline it now so we at least get > > > enum { > > > <alphabetical> > > > <everything else> > > > } > > > > I'm a bit concerned that when a developer sees an alphabetical list, they'll > be > > tempted to insert their new entry in the "proper" place in the middle. So > maybe > > a random_shuffle would be better :D > > I was actually toying with the same idea... and it might not be bad. But we > shouldn't leave this section in there; otherwise it forces new entries under > this weird section of "not valid permissions". So, we should take out the > section (or, at bare minimum, the comment), and if you leave them in > alphabetical order, maybe put a comment at the end saying "Initial batch as of > <date>; new entries below." > > Also, I know we have presubmits for things like ExtensionFunction histograms - > might be worth looking into if we can just give it another source file/enum to > protect? Good idea! I've added a presubmit check for extensions/common, essentially copied from extensions/browser. I've also removed the "not valid permissions" comment, and updated the comment at the top accordingly. https://codereview.chromium.org/1094873002/diff/40001/extensions/common/PRESU... File extensions/common/PRESUBMIT.py (right): https://codereview.chromium.org/1094873002/diff/40001/extensions/common/PRESU... extensions/common/PRESUBMIT.py:31: results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) extensions/browser has this format check, I think it makes sense to have it for extensions/common as well. WDYT?
lgtm, but you'll need a metrics owner. And might be worth letting kalman@ know that you're giving him ownership of a bunch of new histos (even though he owns the ones these are replacing). https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_service.cc:1015: std::string histogram_name_base = On 2015/04/29 11:24:02, Marc Treib wrote: > On 2015/04/28 16:16:36, Devlin wrote: > > On 2015/04/28 12:31:32, Marc Treib wrote: > > > On 2015/04/27 17:58:03, Devlin wrote: > > > > Is there a reason to record both 2 and 3? (It doesn't look like we bother > > > > recording the original anymore...) > > > > > > I just didn't want to break the existing ones until I'm sure the new ones > > > fulfill everyone's needs. After having both for a while (say, a week or two > on > > > Dev at least), I'll remove the old stuff. > > > > Yeah, I understand the motivation, but I'm not sure it's right to "pollute" > the > > old, if we have our concerns at all. The alternative it to pass in a bool for > > whether or not to record old metrics, and only record if we're not using the > new > > system. > > > > I'm fine either way, though - just making sure you've thought of it. Use your > > best judgment. :) > > What do you mean by "pollute"? The old histograms shouldn't change at all with > this CL. Ah, misread part of this. Nevermind; should be fine. https://codereview.chromium.org/1094873002/diff/40001/extensions/common/PRESU... File extensions/common/PRESUBMIT.py (right): https://codereview.chromium.org/1094873002/diff/40001/extensions/common/PRESU... extensions/common/PRESUBMIT.py:31: results += input_api.canned_checks.CheckPatchFormatted(input_api, output_api) On 2015/04/29 11:24:02, Marc Treib wrote: > extensions/browser has this format check, I think it makes sense to have it for > extensions/common as well. WDYT? Eh, probably, as long as it's not a commit blocker (which it isn't here).
Thanks for the review! Yup, the plan was to loop in kalman once you're satisfied, so I'll do that now. I'll add a metrics owner afterwards :)
treib@chromium.org changed reviewers: + kalman@chromium.org
+kalman: FYI, I've added "Extensions.Permissions_*3" histograms to replace the "..2" ones (which are still there for now, but I'm planning to remove them soon-ish). The new histograms record APIPermission::IDs instead of PermissionMessage::IDs, because those will go away at some point. This is a heads-up, since you'll be the owner of the new histograms, and also your chance to voice any concerns :)
thanks for the heads-up. just a couple of comments to make it easier for me to own these histograms. you don't need my lgtm. https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/crx_installer.cc:669: std::string histogram_name = user_initiated ? "InstallCancel" change std::string to a const char* and then remove the c_str() from it below? https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1018: base::StringPrintf("Extensions.Permissions_%s2", histogram), I can see this change get a little bit frustrating. It's nice to be able to see a histogram in the dashboard and be able to grep for that precise string. Now, it's not possible. but I wouldn't want to force callers to give both a "2" and a "3" version in the same place. how about mentioning that in every histogram description? That to grep for this, look for calls to RecordPermissionMessagesHistogram with the argument "<whatever it should be>". https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1032: could you add some comments in this method explaining why there is both 2 and 3, file a bug to remove 2, and reference it here?
https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/crx_installer.cc (right): https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/crx_installer.cc:669: std::string histogram_name = user_initiated ? "InstallCancel" On 2015/04/30 16:31:32, kalman wrote: > change std::string to a const char* and then remove the c_str() from it below? Done. https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1018: base::StringPrintf("Extensions.Permissions_%s2", histogram), On 2015/04/30 16:31:32, kalman wrote: > I can see this change get a little bit frustrating. It's nice to be able to see > a histogram in the dashboard and be able to grep for that precise string. Now, > it's not possible. > > but I wouldn't want to force callers to give both a "2" and a "3" version in the > same place. > > how about mentioning that in every histogram description? That to grep for this, > look for calls to RecordPermissionMessagesHistogram with the argument "<whatever > it should be>". Done. https://codereview.chromium.org/1094873002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1032: On 2015/04/30 16:31:32, kalman wrote: > could you add some comments in this method explaining why there is both 2 and 3, > file a bug to remove 2, and reference it here? Done.
treib@chromium.org changed reviewers: + mpearson@chromium.org
+mpearson for tools/metrics/histograms. PTAL!
https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9453: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> It would be nice to be clear in all the descriptions below whether installs and disables and whatnot transmitted via sync count? For instance, if an extension updates itself and gets autodisables itself, then I load a browser on another machine, and my extension there is disabled, did it get disabled due to the syncing the disabling earlier or getting autodisable again? In other words, do I see another emission to this histogram? Likewise for most of the rest of the histograms. https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9459: + extension upgrade). This implies it's recorded _only_ during the disable event. Is that right? Also, do you mean "when" -> "immediately before"? https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9477: + was aborted, not including installation errors and user cancels. Can you give an example of an aborted installation that has no error and wasn't canceled by the user? What causes these other aborts? ditto with ReEnableAbort3 and WebStoreInstallAbort3 and the .Permissions_ versions as well https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9495: + loaded. add: (which happens at profile open). (right?) also correct .Permissions_Load3 too https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10051: + due to a permission increase (e.g., after an extension upgrade). To grep for optional nit (I think this is clearer): grep for this histogram -> find places where this histogram may be emitted (ditto everywhere below) https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10054: + AutoDisable2. All these grep comments are wrong because you append the "2" or "3" in the body of RecordPermissionMessagesHistogram(). It's not a call with that as an argument.
https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9453: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> On 2015/05/04 18:36:47, Mark P wrote: > It would be nice to be clear in all the descriptions below whether installs and > disables and whatnot transmitted via sync count? For instance, if an extension > updates itself and gets autodisables itself, then I load a browser on another > machine, and my extension there is disabled, did it get disabled due to the > syncing the disabling earlier or getting autodisable again? In other words, do > I see another emission to this histogram? Likewise for most of the rest of the > histograms. Hm, from a quick look at the code, it seems like it would be recorded again, but I don't claim to understand all the intricacies of extension syncing. I just copied these descriptions from the old "2" histograms. Benjamin/Devlin, do you know? https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9459: + extension upgrade). On 2015/05/04 18:36:46, Mark P wrote: > This implies it's recorded _only_ during the disable event. Is that right? Uh.. yes? When else would it be recorded? > Also, do you mean "when" -> "immediately before"? Technically, it's immediately after the "disabled" flag is set, but does it matter? It's somewhere in the middle of the "check for permission increase and disable if necessary" flow. https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9477: + was aborted, not including installation errors and user cancels. On 2015/05/04 18:36:46, Mark P wrote: > Can you give an example of an aborted installation that has no error and wasn't > canceled by the user? What causes these other aborts? Seems like the only case when this happens is when the parent window of the confirmation dialog goes away. Added to the descriptions. > ditto with ReEnableAbort3 and WebStoreInstallAbort3 and the .Permissions_ > versions as well https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9495: + loaded. On 2015/05/04 18:36:46, Mark P wrote: > add: > (which happens at profile open). > > (right?) > > also correct .Permissions_Load3 too Done. https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10051: + due to a permission increase (e.g., after an extension upgrade). To grep for On 2015/05/04 18:36:46, Mark P wrote: > optional nit (I think this is clearer): > grep for this histogram > -> > find places where this histogram may be emitted > > (ditto everywhere below) Done. https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10054: + AutoDisable2. On 2015/05/04 18:36:46, Mark P wrote: > All these grep comments are wrong because you append the "2" or "3" in the body > of RecordPermissionMessagesHistogram(). It's not a call with that as an > argument. Thanks, good catch! Fixed.
https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9453: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> On 2015/05/05 11:49:21, Marc Treib wrote: > On 2015/05/04 18:36:47, Mark P wrote: > > It would be nice to be clear in all the descriptions below whether installs > and > > disables and whatnot transmitted via sync count? For instance, if an > extension > > updates itself and gets autodisables itself, then I load a browser on another > > machine, and my extension there is disabled, did it get disabled due to the > > syncing the disabling earlier or getting autodisable again? In other words, > do > > I see another emission to this histogram? Likewise for most of the rest of > the > > histograms. > > Hm, from a quick look at the code, it seems like it would be recorded again, but > I don't claim to understand all the intricacies of extension syncing. I just > copied these descriptions from the old "2" histograms. > Benjamin/Devlin, do you know? The histograms do nothing intelligent with regards to syncing, so it could be recorded twice. And re the specific example Mark gave, I think it's a race condition between syncing the extension disable and auto-updating the extension - and I don't think we really handle that in a sophisticated way (I think whatever happens first, wins - which will *probably* be sync). Ben, does that sound right to you?
histograms.xml is almost ready https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9453: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> On 2015/05/05 15:34:02, Devlin wrote: > On 2015/05/05 11:49:21, Marc Treib wrote: > > On 2015/05/04 18:36:47, Mark P wrote: > > > It would be nice to be clear in all the descriptions below whether installs > > and > > > disables and whatnot transmitted via sync count? For instance, if an > > extension > > > updates itself and gets autodisables itself, then I load a browser on > another > > > machine, and my extension there is disabled, did it get disabled due to the > > > syncing the disabling earlier or getting autodisable again? In other words, > > do > > > I see another emission to this histogram? Likewise for most of the rest of > > the > > > histograms. > > > > Hm, from a quick look at the code, it seems like it would be recorded again, > but > > I don't claim to understand all the intricacies of extension syncing. I just > > copied these descriptions from the old "2" histograms. > > Benjamin/Devlin, do you know? > > The histograms do nothing intelligent with regards to syncing, so it could be > recorded twice. And re the specific example Mark gave, I think it's a race > condition between syncing the extension disable and auto-updating the extension > - and I don't think we really handle that in a sophisticated way (I think > whatever happens first, wins - which will *probably* be sync). Ben, does that > sound right to you? Assuming Ben agrees with this understanding, I think it would be good to add something to relevant histograms alluding to this race condition. For this histogram, for instance, a message like This can be reported multiple times for the same synced user on different browser installs depending on whether the synced deletion gets applied before or after the extension auto-update mechanism realizes the permission increase. is probably appropriate to remind people. https://codereview.chromium.org/1094873002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9505: + loaded (which happens at profile open). To find places where this histogram Does this also happen at install time? if so, add "or extension install" here and
> Assuming Ben agrees with this understanding, Mmm I didn't look into this CL with enough detail to see a race condition, I just wanted to make sure the UMA was greppable. I'll defer to Devlin. I do like words in UMA though, having recently spent time reading UMA.
On 2015/05/06 21:22:46, kalman wrote: > > Assuming Ben agrees with this understanding, > > Mmm I didn't look into this CL with enough detail to see a race condition, I > just wanted to make sure the UMA was greppable. I'll defer to Devlin. I do like > words in UMA though, having recently spent time reading UMA. This CL doesn't change whether there's a race condition. Looks like there was one before, and still is.
https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9453: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> On 2015/05/06 21:08:51, Mark P wrote: > On 2015/05/05 15:34:02, Devlin wrote: > > On 2015/05/05 11:49:21, Marc Treib wrote: > > > On 2015/05/04 18:36:47, Mark P wrote: > > > > It would be nice to be clear in all the descriptions below whether > installs > > > and > > > > disables and whatnot transmitted via sync count? For instance, if an > > > extension > > > > updates itself and gets autodisables itself, then I load a browser on > > another > > > > machine, and my extension there is disabled, did it get disabled due to > the > > > > syncing the disabling earlier or getting autodisable again? In other > words, > > > do > > > > I see another emission to this histogram? Likewise for most of the rest > of > > > the > > > > histograms. > > > > > > Hm, from a quick look at the code, it seems like it would be recorded again, > > but > > > I don't claim to understand all the intricacies of extension syncing. I just > > > copied these descriptions from the old "2" histograms. > > > Benjamin/Devlin, do you know? > > > > The histograms do nothing intelligent with regards to syncing, so it could be > > recorded twice. And re the specific example Mark gave, I think it's a race > > condition between syncing the extension disable and auto-updating the > extension > > - and I don't think we really handle that in a sophisticated way (I think > > whatever happens first, wins - which will *probably* be sync). Ben, does that > > sound right to you? > > Assuming Ben agrees with this understanding, I think it would be good to add > something to relevant histograms alluding to this race condition. For this > histogram, for instance, a message like > This can be reported multiple times for the same synced user on different > browser installs depending on whether the synced deletion gets applied before or > after the extension auto-update mechanism realizes the permission increase. > is probably appropriate to remind people. I've added "this is per-device" text in the (I hope) appropriate places, i.e. Install and Uninstall are always per-device (but WebStoreInstall isn't), and AutoDisable has the race condition. https://codereview.chromium.org/1094873002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:9505: + loaded (which happens at profile open). To find places where this histogram On 2015/05/06 21:08:52, Mark P wrote: > Does this also happen at install time? > if so, add "or extension install" here and Done.
histograms.xml lgtm once you fix the one remaining comment Thank you for your patience. Hopefully the clear and precise histogram descriptions will serve you well over time. --mark https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9569: + WebStoreInstall. Contrary to the non-WebStore HasPermissions_Install3 I don't see a non-webstore HasPermissions_Install3 Did you mean Contrary to the more-general HasPermissions_Install3 histogram ... i.e., non-WebStore -> more-general ditto in the one occurrence below
https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9569: + WebStoreInstall. Contrary to the non-WebStore HasPermissions_Install3 On 2015/05/07 17:29:45, Mark P wrote: > I don't see a non-webstore HasPermissions_Install3 > Did you mean > Contrary to the more-general HasPermissions_Install3 histogram ... > i.e., non-WebStore -> more-general > > ditto in the one occurrence below Yup, I just meant to say "the version of this that doesn't have "WebStore" in the name". Done.
The CQ bit was checked by treib@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org, kalman@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/1094873002/#ps140001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1094873002/140001
On 2015/05/07 17:29:45, Mark P wrote: > histograms.xml lgtm once you fix the one remaining comment > > Thank you for your patience. Hopefully the clear and precise histogram > descriptions will serve you well over time. > > --mark > > https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1094873002/diff/100001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:9569: + WebStoreInstall. Contrary to > the non-WebStore HasPermissions_Install3 > I don't see a non-webstore HasPermissions_Install3 > Did you mean > Contrary to the more-general HasPermissions_Install3 histogram ... > i.e., non-WebStore -> more-general > > ditto in the one occurrence below ...and thank you for the detailed comments! The histogram descriptions are much clearer now :)
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/2e0517f9e7eaf7901a00d8e475c4cf37c534f9e9 Cr-Commit-Position: refs/heads/master@{#328874}
Message was sent while issue was closed.
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:9469: +<histogram name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> Seems like these histograms are not in alphabetical order now, which should have failed presubmits. 1. Could you fix? Since this fails validation, this is currently blocking histogram updates on our dashboards. 2. Any idea why you didn't see presubmit warnings for this? Did you bypass them? Did you land it via some workflow that doesn't run presubmits? (Just trying to understand what happened so we can fix it for the future.)
Message was sent while issue was closed.
On 2015/05/08 14:11:25, Alexei Svitkine wrote: > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > File tools/metrics/histograms/histograms.xml (right): > > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > tools/metrics/histograms/histograms.xml:9469: +<histogram > name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> > Seems like these histograms are not in alphabetical order now, which should have > failed presubmits. > > 1. Could you fix? Since this fails validation, this is currently blocking > histogram updates on our dashboards. > 2. Any idea why you didn't see presubmit warnings for this? Did you bypass them? > Did you land it via some workflow that doesn't run presubmits? (Just trying to > understand what happened so we can fix it for the future.) chrisha is fixing in https://codereview.chromium.org/1122863005/. did TBR bypass the presubmit checks in the CQ?
Message was sent while issue was closed.
On 2015/05/08 14:15:48, grt wrote: > On 2015/05/08 14:11:25, Alexei Svitkine wrote: > > > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > > File tools/metrics/histograms/histograms.xml (right): > > > > > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > > tools/metrics/histograms/histograms.xml:9469: +<histogram > > name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> > > Seems like these histograms are not in alphabetical order now, which should > have > > failed presubmits. > > > > 1. Could you fix? Since this fails validation, this is currently blocking > > histogram updates on our dashboards. > > 2. Any idea why you didn't see presubmit warnings for this? Did you bypass > them? > > Did you land it via some workflow that doesn't run presubmits? (Just trying to > > understand what happened so we can fix it for the future.) > > chrisha is fixing in https://codereview.chromium.org/1122863005/. > > did TBR bypass the presubmit checks in the CQ? Woah, sorry about that! It's been fixed already though, right? I didn't explicitly bypass any presubmit checks. I guess the TBR might have bypassed the onsubmit check in the CQ, but shouldn't an onupload check also have fired? I have seen that one before, don't know why it wouldn't have fired here.
Message was sent while issue was closed.
Would you mind trying to see if you can repro the onupload check not firing? (don't actually commit, but try in another cl just when uploading) If we can figure out what actually went wrong, then we can make sure it doesn't happen again. On Fri, May 8, 2015 at 11:53 AM, <treib@chromium.org> wrote: > On 2015/05/08 14:15:48, grt wrote: > >> On 2015/05/08 14:11:25, Alexei Svitkine wrote: >> > >> > > > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > >> > File tools/metrics/histograms/histograms.xml (right): >> > >> > >> > > > https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... > >> > tools/metrics/histograms/histograms.xml:9469: +<histogram >> > name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> >> > Seems like these histograms are not in alphabetical order now, which >> should >> have >> > failed presubmits. >> > >> > 1. Could you fix? Since this fails validation, this is currently >> blocking >> > histogram updates on our dashboards. >> > 2. Any idea why you didn't see presubmit warnings for this? Did you >> bypass >> them? >> > Did you land it via some workflow that doesn't run presubmits? (Just >> trying >> > to > >> > understand what happened so we can fix it for the future.) >> > > chrisha is fixing in https://codereview.chromium.org/1122863005/. >> > > did TBR bypass the presubmit checks in the CQ? >> > > Woah, sorry about that! It's been fixed already though, right? > > I didn't explicitly bypass any presubmit checks. I guess the TBR might have > bypassed the onsubmit check in the CQ, but shouldn't an onupload check > also have > fired? I have seen that one before, don't know why it wouldn't have fired > here. > > https://codereview.chromium.org/1094873002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
I tried, and could NOT reproduce the missing onupload check. But looking at the histograms.xml diff in my CL, the histograms are not actually out-of-order. It looks like this was a mis-applied patch: E.g. Extensions.Permissions_AutoDisable3 should have been inserted after Extensions.Permissions_AutoDisable2, but was actually inserted between Extensions.Permissions_AutoDisable and Extensions.Permissions_AutoDisable2. That was possible because the three lines of context in the diff weren't enough to uniquely identify the insertion point - AutoDisable and AutoDisable2 had the same owners and description. So, not sure what we could do to make sure this doesn't happen again, but it should be a fairly unlikely scenario anyway: There need to be histograms with identical descriptions next to each other, AND another CL needs to add a histogram while this one is in the CQ, so that the line numbers for the patch change. On Fri, May 8, 2015 at 9:19 AM Alexei Svitkine <asvitkine@chromium.org> wrote: > Would you mind trying to see if you can repro the onupload check not > firing? (don't actually commit, but try in another cl just when uploading) > > If we can figure out what actually went wrong, then we can make sure it > doesn't happen again. > > On Fri, May 8, 2015 at 11:53 AM, <treib@chromium.org> wrote: > >> On 2015/05/08 14:15:48, grt wrote: >> >>> On 2015/05/08 14:11:25, Alexei Svitkine wrote: >>> > >>> >> >> >> https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... >> >>> > File tools/metrics/histograms/histograms.xml (right): >>> > >>> > >>> >> >> >> https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... >> >>> > tools/metrics/histograms/histograms.xml:9469: +<histogram >>> > name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> >>> > Seems like these histograms are not in alphabetical order now, which >>> should >>> have >>> > failed presubmits. >>> > >>> > 1. Could you fix? Since this fails validation, this is currently >>> blocking >>> > histogram updates on our dashboards. >>> > 2. Any idea why you didn't see presubmit warnings for this? Did you >>> bypass >>> them? >>> > Did you land it via some workflow that doesn't run presubmits? (Just >>> trying >>> >> to >> >>> > understand what happened so we can fix it for the future.) >>> >> >> chrisha is fixing in https://codereview.chromium.org/1122863005/. >>> >> >> did TBR bypass the presubmit checks in the CQ? >>> >> >> Woah, sorry about that! It's been fixed already though, right? >> >> I didn't explicitly bypass any presubmit checks. I guess the TBR might >> have >> bypassed the onsubmit check in the CQ, but shouldn't an onupload check >> also have >> fired? I have seen that one before, don't know why it wouldn't have fired >> here. >> >> https://codereview.chromium.org/1094873002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Wow OK - thanks for tracking this down. Pretty disturbing this can happen - but I guess rare enough that it's not worth worrying too much about. On Fri, May 8, 2015 at 1:24 PM, Marc Treib <treib@chromium.org> wrote: > I tried, and could NOT reproduce the missing onupload check. > But looking at the histograms.xml diff in my CL, the histograms are not > actually out-of-order. It looks like this was a mis-applied patch: > E.g. Extensions.Permissions_AutoDisable3 should have been inserted after Extensions.Permissions_AutoDisable2, > but was actually inserted between Extensions.Permissions_AutoDisable and > Extensions.Permissions_AutoDisable2. > That was possible because the three lines of context in the diff weren't > enough to uniquely identify the insertion point - AutoDisable and > AutoDisable2 had the same owners and description. > > So, not sure what we could do to make sure this doesn't happen again, but > it should be a fairly unlikely scenario anyway: There need to be histograms > with identical descriptions next to each other, AND another CL needs to add > a histogram while this one is in the CQ, so that the line numbers for the > patch change. > > On Fri, May 8, 2015 at 9:19 AM Alexei Svitkine <asvitkine@chromium.org> > wrote: > >> Would you mind trying to see if you can repro the onupload check not >> firing? (don't actually commit, but try in another cl just when uploading) >> >> If we can figure out what actually went wrong, then we can make sure it >> doesn't happen again. >> >> On Fri, May 8, 2015 at 11:53 AM, <treib@chromium.org> wrote: >> >>> On 2015/05/08 14:15:48, grt wrote: >>> >>>> On 2015/05/08 14:11:25, Alexei Svitkine wrote: >>>> > >>>> >>> >>> >>> https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... >>> >>>> > File tools/metrics/histograms/histograms.xml (right): >>>> > >>>> > >>>> >>> >>> >>> https://codereview.chromium.org/1094873002/diff/140001/tools/metrics/histogra... >>> >>>> > tools/metrics/histograms/histograms.xml:9469: +<histogram >>>> > name="Extensions.HasPermissions_AutoDisable3" enum="Boolean"> >>>> > Seems like these histograms are not in alphabetical order now, which >>>> should >>>> have >>>> > failed presubmits. >>>> > >>>> > 1. Could you fix? Since this fails validation, this is currently >>>> blocking >>>> > histogram updates on our dashboards. >>>> > 2. Any idea why you didn't see presubmit warnings for this? Did you >>>> bypass >>>> them? >>>> > Did you land it via some workflow that doesn't run presubmits? (Just >>>> trying >>>> >>> to >>> >>>> > understand what happened so we can fix it for the future.) >>>> >>> >>> chrisha is fixing in https://codereview.chromium.org/1122863005/. >>>> >>> >>> did TBR bypass the presubmit checks in the CQ? >>>> >>> >>> Woah, sorry about that! It's been fixed already though, right? >>> >>> I didn't explicitly bypass any presubmit checks. I guess the TBR might >>> have >>> bypassed the onsubmit check in the CQ, but shouldn't an onupload check >>> also have >>> fired? I have seen that one before, don't know why it wouldn't have >>> fired here. >>> >>> https://codereview.chromium.org/1094873002/ >>> >> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
