Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339393002:
Separate ARC++ Provisioning Histogram into managed and unmanaged (Closed)
|
Created:
4 years, 3 months ago by phweiss Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, asvitkine+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSeparate ARC++ Provisioning Histogram into managed and unmanaged
Deprecate "Arc.Provisioning.Result" and replace it by
"Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged"
for more detailed feedback.
BUG=647670
BUG=b/31442315
Committed: https://crrev.com/1ca52852f992b97cf3bfefff7e70a605ed2295bc
Cr-Commit-Position: refs/heads/master@{#420322}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Build histogram name #
Total comments: 1
Messages
Total messages: 25 (13 generated)
The CQ bit was checked by phweiss@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
phweiss@chromium.org changed reviewers: + elijahtaylor@chromium.org, jwd@chromium.org
Hi, ptal jwd: histograms.xml elijahtaylor: arc_* files Thanks, Philipp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
elijahtaylor1 is OOO. I'll take over arc* stuff. https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_optin_uma.cc:27: UMA_HISTOGRAM_ENUMERATION("Arc.Provisioning.Result.Managed", Can you follow the convention in L40 and build the histogram name?
also, is this intended to be merged to any branches? If so, you need a crbug instead of a b/ bug.
alexchau@google.com changed reviewers: + alexchau@google.com
lgtm
On 2016/09/15 16:00:59, Luis Héctor Chávez wrote: > also, is this intended to be merged to any branches? If so, you need a crbug > instead of a b/ bug. I think we should bring it to M53 too, so we can have more data
Thanks for taking over, Luis. M53 is already stable, do we even still merge there? But we can definitely do M54, I'll create the crbug. https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_optin_uma.cc:27: UMA_HISTOGRAM_ENUMERATION("Arc.Provisioning.Result.Managed", On 2016/09/15 16:00:32, Luis Héctor Chávez wrote: > Can you follow the convention in L40 and build the histogram name? Yes, but then we cannot use the macro anymore. I'm not sure which version I like more, since the macro is easier to read, but it doubles the code. In L40, the non-macro code replaces 4 different histograms, so the decision was more clear-cut.
lgtm provided you edit the issue and add the crbug bug before landing. https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2339393002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_optin_uma.cc:27: UMA_HISTOGRAM_ENUMERATION("Arc.Provisioning.Result.Managed", On 2016/09/15 16:55:51, phweiss wrote: > On 2016/09/15 16:00:32, Luis Héctor Chávez wrote: > > Can you follow the convention in L40 and build the histogram name? > > Yes, but then we cannot use the macro anymore. I'm not sure which version I like > more, since the macro is easier to read, but it doubles the code. In L40, the > non-macro code replaces 4 different histograms, so the decision was more > clear-cut. oh that expansion is also gnarlier than the one in UpdateProvisioningTiming :/ I'm not sure now either, so if you feel more comfortable with the previous one, you can go back.
Description was changed from ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=b/31442315 ========== to ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670, b/31442315 ==========
Description was changed from ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670, b/31442315 ========== to ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670 BUG=b/31442315 ==========
Changed description to add the crbug. I'll leave the expanded code as it is. jwd, can you take a look at histograms.xml?
LGTM with fyi comment https://codereview.chromium.org/2339393002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2339393002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1240: +</histogram> You could use suffixes for this, you'd need to keep the original histogram intact though, so you may not want to. If you're planning on doing this managed/unmanaged splitting of other histograms though, it may be worth it to do suffixes. If you want to keep the old one obsolete, you can create a new histogram to base your suffixes off of. Leaving it up to you.
The CQ bit was checked by phweiss@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexchau@google.com Link to the patchset: https://codereview.chromium.org/2339393002/#ps20001 (title: "Build histogram name")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670 BUG=b/31442315 ========== to ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670 BUG=b/31442315 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670 BUG=b/31442315 ========== to ========== Separate ARC++ Provisioning Histogram into managed and unmanaged Deprecate "Arc.Provisioning.Result" and replace it by "Arc.Provisioning.Result.Managed" and "Arc.Provisioning.Result.Unmanaged" for more detailed feedback. BUG=647670 BUG=b/31442315 Committed: https://crrev.com/1ca52852f992b97cf3bfefff7e70a605ed2295bc Cr-Commit-Position: refs/heads/master@{#420322} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1ca52852f992b97cf3bfefff7e70a605ed2295bc Cr-Commit-Position: refs/heads/master@{#420322} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
