Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2704383006:
arc: Handle metrics for Auth operations. (Closed)
|
Created:
3 years, 10 months ago by khmel Modified:
3 years, 10 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, viettrungluu+watch_chromium.org, hidehiko+watch_chromium.org, yzshen+watch_chromium.org, khmel+watch_chromium.org, asvitkine+watch_chromium.org, Aaron Boodman, lhchavez+watch_chromium.org, oshima+watch_chromium.org, darin (slow to review), abarth-chromium, davemoore+watch_chromium.org, qsr+mojo_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Handle metrics for Auth operations.
This handles messages with Auth operation metrics and add it to
UMA.
TEST=Locally, new metrics appear in chrome://histograms
BUG=b/35707115
BUG=695542
Review-Url: https://codereview.chromium.org/2704383006
Cr-Commit-Position: refs/heads/master@{#452722}
Committed: https://chromium.googlesource.com/chromium/src/+/052c11832f70f11fb220abb005bcf93143d2e4e8
Patch Set 1 #
Total comments: 2
Patch Set 2 : nits #
Total comments: 6
Patch Set 3 : comments addressed #
Total comments: 2
Patch Set 4 : rebased, use direct names for his. tables #Patch Set 5 : update histogram.xml to match change Arc.Auth to ArcAuth #
Total comments: 14
Patch Set 6 : uma comments addressed #
Total comments: 4
Patch Set 7 : nit in histograms.xml #
Messages
Total messages: 29 (11 generated)
khmel@chromium.org changed reviewers: + lhchavez@chromium.org
Hi Luis, PTAL Thank you!
Hi Luis, PTAL Thank you!
The CQ bit was checked by khmel@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...
lgtm with a nit. https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_optin_uma.cc:16: std::string histogram_name = "Arc.Auth."; constexpr char kArcAuthHistogramPrefix[] = "Arc.Auth."; std::string GetAuthHistogramName(const std::string& key) { return kArcAuthHistogramPrefix + key; }
On 2017/02/23 18:26:55, Luis Héctor Chávez wrote: > lgtm with a nit. > > https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc... > File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): > > https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc... > chrome/browser/chromeos/arc/arc_optin_uma.cc:16: std::string histogram_name = > "Arc.Auth."; > constexpr char kArcAuthHistogramPrefix[] = "Arc.Auth."; > > std::string GetAuthHistogramName(const std::string& key) { > return kArcAuthHistogramPrefix + key; > } also, do you intend to merge this to a release branch? if so, replace the buganizer bug with a crbug.com bug.
Description was changed from ========== arc: Handle metrics for Auth operations. This handles messages with Auth operation metrics and add it to UMA. TEST=Locally, new metrics appear in chrome://histograms BUG= 35707115 ========== to ========== arc: Handle metrics for Auth operations. This handles messages with Auth operation metrics and add it to UMA. TEST=Locally, new metrics appear in chrome://histograms BUG=b/35707115 BUG=695542 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
khmel@chromium.org changed reviewers: + dcheng@chromium.org, isherman@chromium.org
Thank you Luis for review.
Adding
dcheng@ - components/arc/common/auth.mojom
isherman@ - tools/metrics/histograms/histograms.xml
c/b/chromeos/arc/arc_optin_uma.cc
PTAL
https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_optin_uma.cc (right):
https://codereview.chromium.org/2704383006/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_optin_uma.cc:16: std::string histogram_name =
"Arc.Auth.";
On 2017/02/23 18:26:55, Luis Héctor Chávez wrote:
> constexpr char kArcAuthHistogramPrefix[] = "Arc.Auth.";
>
> std::string GetAuthHistogramName(const std::string& key) {
> return kArcAuthHistogramPrefix + key;
> }
Done.
https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:18: return kArcAuthHistogramPrefix + key; Just add the Arc.Auth. prefix to the UMAs, it'll be easier to grep that way. https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:64: void UpdateAuthTiming(const std::string& key, And then pass this as const char* to save some temporary string constructions https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:65: const base::TimeDelta& elapsed_time) { Nit: pass by value
Thank you Daniel for quick review. PTAL updated version. https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:18: return kArcAuthHistogramPrefix + key; On 2017/02/23 19:50:19, dcheng wrote: > Just add the Arc.Auth. prefix to the UMAs, it'll be easier to grep that way. Done. https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:64: void UpdateAuthTiming(const std::string& key, On 2017/02/23 19:50:19, dcheng wrote: > And then pass this as const char* to save some temporary string constructions Done. https://codereview.chromium.org/2704383006/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:65: const base::TimeDelta& elapsed_time) { On 2017/02/23 19:50:19, dcheng wrote: > Nit: pass by value Done.
https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:184: UpdateAuthTiming("NetworkWaitTime", I don't see Arc.Auth. prefix here, maybe wrong PS got uploaded?
https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:184: UpdateAuthTiming("NetworkWaitTime", On 2017/02/23 20:12:36, dcheng wrote: > I don't see Arc.Auth. prefix here, maybe wrong PS got uploaded? ArcAuth. is added at UpdateAuthTiming/UpdateAuthAttempts. NetworkWaitTime is used to construct ArcAuth.NetworkWaitTime. Idea of this CL is to define set of 4 UMAs started with ArcAuth.
On 2017/02/23 20:14:53, khmel wrote: > https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_auth_service.cc:184: > UpdateAuthTiming("NetworkWaitTime", > On 2017/02/23 20:12:36, dcheng wrote: > > I don't see Arc.Auth. prefix here, maybe wrong PS got uploaded? > > ArcAuth. is added at UpdateAuthTiming/UpdateAuthAttempts. NetworkWaitTime is > used to construct ArcAuth.NetworkWaitTime. Idea of this CL is to define set of 4 > UMAs started with ArcAuth. Right, I'm saying that we should just include the string directly in the string literal. This saves the construction of a temp string and makes it easier to grep for the UMA in th esource code.
On 2017/02/23 20:37:05, dcheng wrote: > On 2017/02/23 20:14:53, khmel wrote: > > > https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_auth_service.cc (right): > > > > > https://codereview.chromium.org/2704383006/diff/40001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_auth_service.cc:184: > > UpdateAuthTiming("NetworkWaitTime", > > On 2017/02/23 20:12:36, dcheng wrote: > > > I don't see Arc.Auth. prefix here, maybe wrong PS got uploaded? > > > > ArcAuth. is added at UpdateAuthTiming/UpdateAuthAttempts. NetworkWaitTime is > > used to construct ArcAuth.NetworkWaitTime. Idea of this CL is to define set of > 4 > > UMAs started with ArcAuth. > > Right, I'm saying that we should just include the string directly in the string > literal. This saves the construction of a temp string and makes it easier to > grep for the UMA in th esource code. Got you, PTAL new PS
thanks. mojo lgtm
https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:60: ->AddTime(elapsed_time); Please use the convenience wrapper base::UmaHistogramCustomTimes from histogram_functions.h. https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:65: 5 /* maximum */, 6 /* bucket_count */); Please use the convenience wrapper base::UmaHistogramExactLinear or UMA_HISTOGRAM_SPARSE_SLOWLY if you need to record to multiple named histograms. Note that it's not safe to pass a runtime-variable name to most of the UMA_HISTOGRAM macros (the one exception being UMA_HISTOGRAM_SPARSE_SLOWLY). However, it looks like you are actually only ever passing a single histogram name to this function. Why pass in the name as a parameter at all? I'd recommend writing this function as void UpdateAuthAttempts(int32_t num_attempts) { UMA_HISTOGRAM_SPARSE_SLOWLY("ArcAuth.CheckinAttempts", num_attempts); } (Note that I also renamed the generically named "value" to the clearer "num_attempts".) https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1601: + <owner>elijahtaylor@google.com</owner> Can you use an @chromium.org address here? Also, I noticed that Elijah isn't on the review list for this CL. Are you sure that he is comfortable with being signed up to own these? https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1603: + Number of attempts did while waiting for checkin task completed before GMS nit: s/did/done https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1603: + Number of attempts did while waiting for checkin task completed before GMS nit: s/checkin task completed/the check-in task to be completed https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1604: + sign-in. 0 means that device was already checked-in prior to GMS sign-in. Please document when this is recorded -- on success? After each attempt? Something else? https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1611: + Elapsed time waiting for checkin task completed before GMS sign-in. Same grammatical tweak suggestions as above. Also, I'm not quite sure what "before GMS sign-in" means, in this sentence. Is the intent to convey that the checkin task runs at multiple times, and this histogram is only recorded for checkin tasks that run before GMS sign-in? That complete before GMS sign-in? Something else? Please reword this to clarify, if you can.
Thank you Ilya for comments. PTAL https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_optin_uma.cc (right): https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:60: ->AddTime(elapsed_time); On 2017/02/23 21:23:34, Ilya Sherman wrote: > Please use the convenience wrapper base::UmaHistogramCustomTimes from > histogram_functions.h. Done. https://codereview.chromium.org/2704383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_optin_uma.cc:65: 5 /* maximum */, 6 /* bucket_count */); On 2017/02/23 21:23:34, Ilya Sherman wrote: > Please use the convenience wrapper base::UmaHistogramExactLinear or > UMA_HISTOGRAM_SPARSE_SLOWLY if you need to record to multiple named histograms. > Note that it's not safe to pass a runtime-variable name to most of the > UMA_HISTOGRAM macros (the one exception being UMA_HISTOGRAM_SPARSE_SLOWLY). > > However, it looks like you are actually only ever passing a single histogram > name to this function. Why pass in the name as a parameter at all? I'd > recommend writing this function as > > void UpdateAuthAttempts(int32_t num_attempts) { > UMA_HISTOGRAM_SPARSE_SLOWLY("ArcAuth.CheckinAttempts", num_attempts); > } > > (Note that I also renamed the generically named "value" to the clearer > "num_attempts".) Thanks for detailed comment. I reserved it for possible extension and to keep it aligned with prev function. I am ok to use name here. Done https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1601: + <owner>elijahtaylor@google.com</owner> On 2017/02/23 21:23:35, Ilya Sherman wrote: > Can you use an @chromium.org address here? Also, I noticed that Elijah isn't on > the review list for this CL. Are you sure that he is comfortable with being > signed up to own these? I changed to @chromium.org (however in many other existing we use @google.com for ARC). Yes, we discussed what need to be done with Elijah. He is also in cc list. https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1603: + Number of attempts did while waiting for checkin task completed before GMS On 2017/02/23 21:23:34, Ilya Sherman wrote: > nit: s/did/done Done. https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1603: + Number of attempts did while waiting for checkin task completed before GMS On 2017/02/23 21:23:34, Ilya Sherman wrote: > nit: s/did/done Done. https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1604: + sign-in. 0 means that device was already checked-in prior to GMS sign-in. On 2017/02/23 21:23:35, Ilya Sherman wrote: > Please document when this is recorded -- on success? After each attempt? > Something else? Done. https://codereview.chromium.org/2704383006/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:1611: + Elapsed time waiting for checkin task completed before GMS sign-in. On 2017/02/23 21:23:35, Ilya Sherman wrote: > Same grammatical tweak suggestions as above. Also, I'm not quite sure what > "before GMS sign-in" means, in this sentence. Is the intent to convey that the > checkin task runs at multiple times, and this histogram is only recorded for > checkin tasks that run before GMS sign-in? That complete before GMS sign-in? > Something else? Please reword this to clarify, if you can. I removed mentioning GMS sign-in. It is actually not required.
Metrics LGTM % nits: https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:1604: + before GMS sign-in. 0 means that device was already checked-in. This is Same question about GMS sign-in here, fwiw. https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:1627: + <summary>Elapsed time waiting for GMS sign-in completed.</summary> nit: s/completed/to complete.
Thank you for reviewing! https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:1604: + before GMS sign-in. 0 means that device was already checked-in. This is On 2017/02/23 22:23:22, Ilya Sherman wrote: > Same question about GMS sign-in here, fwiw. Sorry, missed this. https://codereview.chromium.org/2704383006/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:1627: + <summary>Elapsed time waiting for GMS sign-in completed.</summary> On 2017/02/23 22:23:22, Ilya Sherman wrote: > nit: s/completed/to complete. Done.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, dcheng@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2704383006/#ps120001 (title: "nit in histograms.xml")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1487889702044610,
"parent_rev": "6aa7cec3e8fbc175ef123f2d6f6ce72b0e98f4fa", "commit_rev":
"052c11832f70f11fb220abb005bcf93143d2e4e8"}
Message was sent while issue was closed.
Description was changed from ========== arc: Handle metrics for Auth operations. This handles messages with Auth operation metrics and add it to UMA. TEST=Locally, new metrics appear in chrome://histograms BUG=b/35707115 BUG=695542 ========== to ========== arc: Handle metrics for Auth operations. This handles messages with Auth operation metrics and add it to UMA. TEST=Locally, new metrics appear in chrome://histograms BUG=b/35707115 BUG=695542 Review-Url: https://codereview.chromium.org/2704383006 Cr-Commit-Position: refs/heads/master@{#452722} Committed: https://chromium.googlesource.com/chromium/src/+/052c11832f70f11fb220abb005bc... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/052c11832f70f11fb220abb005bc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
