Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
211663004:
Cards Clicked Metrics (Closed)
|
Created:
6 years, 9 months ago by robliao Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org, asvitkine+watch_chromium.org, jar (doing other things) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionCards Clicked Metrics
Provides metrics on the types of cards clicked.
BUG=164227
R=isherman@chromium.org, skare@chromium.org, xiyuan@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260028
Patch Set 1 #
Total comments: 2
Patch Set 2 : Increase Total #
Total comments: 10
Patch Set 3 : CR Feedback #Patch Set 4 : Use the new sparse histogram support. #
Messages
Total messages: 19 (0 generated)
lgtm https://codereview.chromium.org/211663004/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:269: chrome.metricsPrivate.recordValue(metricDescription, cardTypeId); Should we range-check cardTypeId<=CARD_TYPES_TOTAL here? I don't think we need to; the callee should. Does that cause an error to be printed, or does it silently fail?
isherman@chromium.org: Please review changes in tools/metrics/histograms/histograms.xml Thanks! https://codereview.chromium.org/211663004/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:269: chrome.metricsPrivate.recordValue(metricDescription, cardTypeId); I remember a series of crashes surrounding this code. Do you happen to recall what that involved? On 2014/03/25 23:55:39, Travis Skare wrote: > Should we range-check cardTypeId<=CARD_TYPES_TOTAL here? > > I don't think we need to; the callee should. Does that cause an error to be > printed, or does it silently fail?
https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:238: var CARD_TYPES_TOTAL = 300; Hmm, can you use a sparse histogram rather than allocating memory for 300 buckets, most of which will probably be empty? https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6836: + <summary>Types of cards which received an notification click.</summary> nit: "an notification" -> "a notification" https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32836: +</enum> Do these need to be three separate enums, or can there just be a single GoogleNowCard enum?
https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:238: var CARD_TYPES_TOTAL = 300; Got docs on how to do this? I only see histogram-log and histogram-linear as options. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... On 2014/03/26 00:32:50, Ilya Sherman wrote: > Hmm, can you use a sparse histogram rather than allocating memory for 300 > buckets, most of which will probably be empty? https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:6836: + <summary>Types of cards which received an notification click.</summary> On 2014/03/26 00:32:50, Ilya Sherman wrote: > nit: "an notification" -> "a notification" Done. https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32836: +</enum> On 2014/03/26 00:32:50, Ilya Sherman wrote: > Do these need to be three separate enums, or can there just be a single > GoogleNowCard enum? We have two variables we need to track, type and button ID. If there's a two dimensional histogram, we could use that.
https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:238: var CARD_TYPES_TOTAL = 300; You might need to expand the metricsPrivate API to support this -- I'm not very familiar with what all this API supports. In C++ code, the macro to call is UMA_HISTOGRAM_SPARSE_SLOWLY. On 2014/03/26 00:42:53, robliao wrote: > Got docs on how to do this? I only see histogram-log and histogram-linear as > options. > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > On 2014/03/26 00:32:50, Ilya Sherman wrote: > > Hmm, can you use a sparse histogram rather than allocating memory for 300 > > buckets, most of which will probably be empty? > https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32836: +</enum> On 2014/03/26 00:42:53, robliao wrote: > On 2014/03/26 00:32:50, Ilya Sherman wrote: > > Do these need to be three separate enums, or can there just be a single > > GoogleNowCard enum? > > We have two variables we need to track, type and button ID. If there's a two > dimensional histogram, we could use that. I don't understand what you mean. Say you're interested in data about the TRAFFIC card. Is it going to have a different value in each of these enums? If so, why? It would help if these enums had at least *some* labels, rather than being completely empty. I understand that you want to expand them later, but presumably some are already known -- right?
https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/211663004/diff/30001/chrome/browser/resources... chrome/browser/resources/google_now/background.js:238: var CARD_TYPES_TOTAL = 300; Done. https://codereview.chromium.org/213433003/ On 2014/03/26 03:55:41, Ilya Sherman wrote: > You might need to expand the metricsPrivate API to support this -- I'm not very > familiar with what all this API supports. In C++ code, the macro to call is > UMA_HISTOGRAM_SPARSE_SLOWLY. > > On 2014/03/26 00:42:53, robliao wrote: > > Got docs on how to do this? I only see histogram-log and histogram-linear as > > options. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > On 2014/03/26 00:32:50, Ilya Sherman wrote: > > > Hmm, can you use a sparse histogram rather than allocating memory for 300 > > > buckets, most of which will probably be empty? > > > https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/211663004/diff/30001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:32836: +</enum> Ah, I see what you mean. Changed. On 2014/03/26 03:55:41, Ilya Sherman wrote: > On 2014/03/26 00:42:53, robliao wrote: > > On 2014/03/26 00:32:50, Ilya Sherman wrote: > > > Do these need to be three separate enums, or can there just be a single > > > GoogleNowCard enum? > > > > We have two variables we need to track, type and button ID. If there's a two > > dimensional histogram, we could use that. > > I don't understand what you mean. Say you're interested in data about the > TRAFFIC card. Is it going to have a different value in each of these enums? If > so, why? It would help if these enums had at least *some* labels, rather than > being completely empty. I understand that you want to expand them later, but > presumably some are already known -- right?
Histograms LGTM, thanks.
xiyuan: Please provide owner approval for the following files. Thanks! chrome/browser/resources/google_now/background.js chrome/browser/resources/google_now/background_unittest.gtestjs chrome/browser/resources/google_now/cards.js
LGTM Why CL description says "Provides telemetry"? Shound't it be "Provide metrics"?
On 2014/03/27 17:33:07, xiyuan wrote: > LGTM > > Why CL description says "Provides telemetry"? Shound't it be "Provide metrics"? In Chrome-speak, yup. Changed.
On 2014/03/27 17:33:48, robliao wrote: > On 2014/03/27 17:33:07, xiyuan wrote: > > LGTM > > > > Why CL description says "Provides telemetry"? Shound't it be "Provide > metrics"? > > In Chrome-speak, yup. Changed. Submitting soon.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/211663004/160001
The CQ bit was unchecked by robliao@chromium.org
The CQ bit was checked by robliao@chromium.org
The CQ bit was unchecked by robliao@chromium.org
Message was sent while issue was closed.
Committed patchset #4 manually as r260028 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
