Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1548283003:
Add P2 UMA metrics for Media Router user actions. (Closed)
|
Created:
4 years, 12 months ago by apacible Modified:
4 years, 11 months ago CC:
chromium-reviews, media-router+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd P2 UMA metrics for Media Router user actions.
This change adds the following metrics:
- The type of route when the user closes it.
- The type of cast source when the user picks one.
- The index of the sink that the user selected in the sink list.
BUG=547977
Committed: https://crrev.com/428f9b854b7a27b76167a2b1805baf008b9622f4
Cr-Commit-Position: refs/heads/master@{#367184}
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Changes per isherman@'s comments. #Patch Set 3 : Fix compiler errors on Windows. #
Total comments: 8
Patch Set 4 : Changes per isherman@'s comments. #Patch Set 5 : Compiler error. #Patch Set 6 : Changes per isherman@'s comments. #Patch Set 7 : #
Messages
Total messages: 23 (10 generated)
Description was changed from ========== p2 mr user action BUG= ========== to ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - Count of times user closes a route -- specifically if it was a local or remote route. BUG=547977 ==========
Description was changed from ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - Count of times user closes a route -- specifically if it was a local or remote route. BUG=547977 ========== to ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - The type of route when the user closes it. - The type of cast source when the user picks one. - The index of the sink that the user selected in the sink list. BUG=547977 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
apacible@chromium.org changed reviewers: + imcheng@chromium.org, isherman@chromium.org
PTAL, thanks! imcheng for everything isherman for histograms.xml OWNER
https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:387: UMA_HISTOGRAM_COUNTS_100("MediaRouter.Ui.Action.StopRoute", route_type); Please use UMA_HISTOGRAM_BOOLEAN if you're only emitting two values. https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:409: index); Optional: It might be reasonable to use UMA_HISTOGRAM_SPARSE_SLOWLY here, capping the logged value at some reasonable upper bound (say, 100). This would both (a) give you more precise counts, and (b) use less memory. https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:421: cast_mode_type); This should be either UMA_HISTOGRAM_ENUMERATION (probably preferred in this case) or UMA_HISTOGRAM_SPARSE_SLOWLY. https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19646: + be non-negative. Histograms don't actually record negative samples, so if it's important for you to test for non-negativity, I'd recommend doing so prior to emitting to the histogram. https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70390: + <int value="4" label="Desktop"/> What happened to 3?
https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:387: UMA_HISTOGRAM_COUNTS_100("MediaRouter.Ui.Action.StopRoute", route_type); On 2015/12/28 23:13:00, Ilya Sherman wrote: > Please use UMA_HISTOGRAM_BOOLEAN if you're only emitting two values. Done. https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:409: index); On 2015/12/28 23:13:00, Ilya Sherman wrote: > Optional: It might be reasonable to use UMA_HISTOGRAM_SPARSE_SLOWLY here, > capping the logged value at some reasonable upper bound (say, 100). This would > both (a) give you more precise counts, and (b) use less memory. Done. https://codereview.chromium.org/1548283003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:421: cast_mode_type); On 2015/12/28 23:13:00, Ilya Sherman wrote: > This should be either UMA_HISTOGRAM_ENUMERATION (probably preferred in this > case) or UMA_HISTOGRAM_SPARSE_SLOWLY. Done. https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:19646: + be non-negative. On 2015/12/28 23:13:00, Ilya Sherman wrote: > Histograms don't actually record negative samples, so if it's important for you > to test for non-negativity, I'd recommend doing so prior to emitting to the > histogram. Removed comment about non-negativity. https://codereview.chromium.org/1548283003/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:70390: + <int value="4" label="Desktop"/> On 2015/12/28 23:13:00, Ilya Sherman wrote: > What happened to 3? I'm following the MediaCastMode values: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... We use powers of two for bitset operations on the WebUI side, so if we add an additional cast mode, it would map to 8.
https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } nit: I think emitting min(index, 100) is probably better than silently dropping values over 100. You can add a label of "100+" in histograms.xml to clarify that this is an overflow bucket. https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: cast_mode_type); If the enum that you're emitting is exponentially spaced rather than linearly spaced, then a sparse histogram is better. UMA_HISTOGRAM_ENUMERATION is dense, i.e. is backed by a vector. This works well for enums that are themselves dense, but not very well for ones that are exponentially spaced (i.e. bitmasks).
https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } On 2015/12/29 01:39:04, Ilya Sherman wrote: > nit: I think emitting min(index, 100) is probably better than silently dropping > values over 100. You can add a label of "100+" in histograms.xml to clarify > that this is an overflow bucket. Switched to using min(index, 100). > add a label of "100+" in histograms.xml Does this mean adding a note that this would be an overflow bucket, or is there a way to specifically relabel the bucket? I didn't find a way to relabel the specific bucket in the comments/existing histograms, but I may have missed it. https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:424: cast_mode_type); On 2015/12/29 01:39:04, Ilya Sherman wrote: > If the enum that you're emitting is exponentially spaced rather than linearly > spaced, then a sparse histogram is better. UMA_HISTOGRAM_ENUMERATION is dense, > i.e. is backed by a vector. This works well for enums that are themselves > dense, but not very well for ones that are exponentially spaced (i.e. bitmasks). Switched.
histograms LGTM https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } On 2015/12/29 02:58:14, apacible wrote: > On 2015/12/29 01:39:04, Ilya Sherman wrote: > > nit: I think emitting min(index, 100) is probably better than silently > dropping > > values over 100. You can add a label of "100+" in histograms.xml to clarify > > that this is an overflow bucket. > > Switched to using min(index, 100). > > > add a label of "100+" in histograms.xml > Does this mean adding a note that this would be an overflow bucket, or is there > a way to specifically relabel the bucket? I didn't find a way to relabel the > specific bucket in the comments/existing histograms, but I may have missed it. I mean associating an enum attribute with the histogram (just in histograms.xml) and providing a label just for the 100th bucket. The enum attribute in the XML file could also be named "labels".
https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } On 2015/12/29 03:10:14, Ilya Sherman wrote: > On 2015/12/29 02:58:14, apacible wrote: > > On 2015/12/29 01:39:04, Ilya Sherman wrote: > > > nit: I think emitting min(index, 100) is probably better than silently > > dropping > > > values over 100. You can add a label of "100+" in histograms.xml to clarify > > > that this is an overflow bucket. > > > > Switched to using min(index, 100). > > > > > add a label of "100+" in histograms.xml > > Does this mean adding a note that this would be an overflow bucket, or is > there > > a way to specifically relabel the bucket? I didn't find a way to relabel the > > specific bucket in the comments/existing histograms, but I may have missed it. > > I mean associating an enum attribute with the histogram (just in histograms.xml) > and providing a label just for the 100th bucket. The enum attribute in the XML > file could also be named "labels". I see. I labeled "99" as "99+" since the positions start at 0.
https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } On 2015/12/29 03:25:16, apacible wrote: > On 2015/12/29 03:10:14, Ilya Sherman wrote: > > On 2015/12/29 02:58:14, apacible wrote: > > > On 2015/12/29 01:39:04, Ilya Sherman wrote: > > > > nit: I think emitting min(index, 100) is probably better than silently > > > dropping > > > > values over 100. You can add a label of "100+" in histograms.xml to > clarify > > > > that this is an overflow bucket. > > > > > > Switched to using min(index, 100). > > > > > > > add a label of "100+" in histograms.xml > > > Does this mean adding a note that this would be an overflow bucket, or is > > there > > > a way to specifically relabel the bucket? I didn't find a way to relabel the > > > specific bucket in the comments/existing histograms, but I may have missed > it. > > > > I mean associating an enum attribute with the histogram (just in > histograms.xml) > > and providing a label just for the 100th bucket. The enum attribute in the > XML > > file could also be named "labels". > > I see. I labeled "99" as "99+" since the positions start at 0. I'm not sure what you mean by "since the positions start at 0". For ease of reasoning, let's imagine that you capped the index as "min(index, 5)". Then you'd emit histogram values as follows: 0 ==> bucket 0 1 ==> bucket 1 2 ==> bucket 2 3 ==> bucket 3 4 ==> bucket 4 5 ==> bucket 5 6 ==> bucket 5 7 ==> bucket 5 ... and so on But, you're suggesting a label of "4+". That doesn't make sense. Are you saying that index is 0-based, whereas you want to emit a 1-based index to the histogram? If so, then you should emit min(index + 1, 100). But, your label should still be "100+" in this case -- otherwise, you'd have both a bucket "99" and a bucket "99+". Or am I misunderstanding something?
Patchset #7 (id:160001) has been deleted
https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc (right): https://codereview.chromium.org/1548283003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/media_router/media_router_webui_message_handler.cc:412: } On 2015/12/29 05:27:44, Ilya Sherman wrote: > On 2015/12/29 03:25:16, apacible wrote: > > On 2015/12/29 03:10:14, Ilya Sherman wrote: > > > On 2015/12/29 02:58:14, apacible wrote: > > > > On 2015/12/29 01:39:04, Ilya Sherman wrote: > > > > > nit: I think emitting min(index, 100) is probably better than silently > > > > dropping > > > > > values over 100. You can add a label of "100+" in histograms.xml to > > clarify > > > > > that this is an overflow bucket. > > > > > > > > Switched to using min(index, 100). > > > > > > > > > add a label of "100+" in histograms.xml > > > > Does this mean adding a note that this would be an overflow bucket, or is > > > there > > > > a way to specifically relabel the bucket? I didn't find a way to relabel > the > > > > specific bucket in the comments/existing histograms, but I may have missed > > it. > > > > > > I mean associating an enum attribute with the histogram (just in > > histograms.xml) > > > and providing a label just for the 100th bucket. The enum attribute in the > > XML > > > file could also be named "labels". > > > > I see. I labeled "99" as "99+" since the positions start at 0. > > I'm not sure what you mean by "since the positions start at 0". For ease of > reasoning, let's imagine that you capped the index as "min(index, 5)". Then > you'd emit histogram values as follows: > > 0 ==> bucket 0 > 1 ==> bucket 1 > 2 ==> bucket 2 > 3 ==> bucket 3 > 4 ==> bucket 4 > 5 ==> bucket 5 > 6 ==> bucket 5 > 7 ==> bucket 5 > ... and so on > > But, you're suggesting a label of "4+". That doesn't make sense. > > Are you saying that index is 0-based, whereas you want to emit a 1-based index > to the histogram? If so, then you should emit min(index + 1, 100). But, your > label should still be "100+" in this case -- otherwise, you'd have both a bucket > "99" and a bucket "99+". > > Or am I misunderstanding something? Oh, you're right. Switched to 100+ for value 100.
lgtm
The CQ bit was checked by apacible@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org Link to the patchset: https://codereview.chromium.org/1548283003/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1548283003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1548283003/180001
Message was sent while issue was closed.
Description was changed from ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - The type of route when the user closes it. - The type of cast source when the user picks one. - The index of the sink that the user selected in the sink list. BUG=547977 ========== to ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - The type of route when the user closes it. - The type of cast source when the user picks one. - The index of the sink that the user selected in the sink list. BUG=547977 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - The type of route when the user closes it. - The type of cast source when the user picks one. - The index of the sink that the user selected in the sink list. BUG=547977 ========== to ========== Add P2 UMA metrics for Media Router user actions. This change adds the following metrics: - The type of route when the user closes it. - The type of cast source when the user picks one. - The index of the sink that the user selected in the sink list. BUG=547977 Committed: https://crrev.com/428f9b854b7a27b76167a2b1805baf008b9622f4 Cr-Commit-Position: refs/heads/master@{#367184} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/428f9b854b7a27b76167a2b1805baf008b9622f4 Cr-Commit-Position: refs/heads/master@{#367184} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
