Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1305453002:
Add UMA stats for Chromoting connection details. (Closed)
DescriptionAdd UMA stats for connection times and durations in the Chromoting plugin.
Also add UMA counters for each connection-state in the web-app.
BUG=502061
Committed: https://crrev.com/49b2fcd7c800c94961c5644b847639dabccdc7e7
Cr-Commit-Position: refs/heads/master@{#344930}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Add Chromoting actions to actions.xml. Fix durations calculations. #
Total comments: 10
Patch Set 3 : Change connection times and durations histogram names. Clean up histograms.xml. #Patch Set 4 : Use enum histogram instead of user actions for connection states. #
Total comments: 14
Patch Set 5 : Include all SessionState enums in UMA enumerated-histogram. Cleanup plugin code. #
Total comments: 14
Patch Set 6 : Use only non-negative values for UMA enumerated histogram. #
Total comments: 8
Patch Set 7 : Include min and max enum in SessionState enum definition. #Patch Set 8 : Rebase. #
Total comments: 1
Messages
Total messages: 43 (19 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
anandc@chromium.org changed reviewers: + sergeyu@chromium.org
Hi Sergey, PTAL. Thank you.
https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:418: (base::Time::Now() - connection_connected_).InMilliseconds(), connection_connected_ is set only if the session was connected, so this will report bogus values if we get to FAILED state while trying to connect. Same for CLOSED. https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.h:292: base::Time connection_connecting_; use base::TimeTicks, instead of base::Time. https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.h:292: base::Time connection_connecting_; nit: rename these to indicate the purpose more clearly. E.g. connection_started_time_, authenticated_time_, connected_time_. https://codereview.chromium.org/1305453002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/client_session.js:585: 'Chromoting.Connections.Connected'); Do we need to register the actions somewhere? I think we do. base/metrics/user_metrics.h says that you need to run tools/metrics/actions/extract_actions.py which is supposed to extract the list from sources. But for JavaScrip it has hardcoded list of metrics: https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/acti... I think you want to add the new metrics into that list.
anandc@chromium.org changed reviewers: + isherman@chromium.org
Thanks Sergey. PTAL at patch-set #2. +isherman@ for review of histograms.xml and actions.xml. https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.cc:418: (base::Time::Now() - connection_connected_).InMilliseconds(), On 2015/08/20 21:05:19, Sergey Ulanov wrote: > connection_connected_ is set only if the session was connected, so this will > report bogus values if we get to FAILED state while trying to connect. Same for > CLOSED. Done. https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.h:292: base::Time connection_connecting_; On 2015/08/20 21:05:19, Sergey Ulanov wrote: > nit: rename these to indicate the purpose more clearly. E.g. > connection_started_time_, authenticated_time_, connected_time_. Done. https://codereview.chromium.org/1305453002/diff/60001/remoting/client/plugin/... remoting/client/plugin/chromoting_instance.h:292: base::Time connection_connecting_; On 2015/08/20 21:05:19, Sergey Ulanov wrote: > use base::TimeTicks, instead of base::Time. Done. https://codereview.chromium.org/1305453002/diff/60001/remoting/webapp/base/js... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/60001/remoting/webapp/base/js... remoting/webapp/base/js/client_session.js:585: 'Chromoting.Connections.Connected'); On 2015/08/20 21:05:19, Sergey Ulanov wrote: > Do we need to register the actions somewhere? I think we do. > > base/metrics/user_metrics.h says that you need to run > tools/metrics/actions/extract_actions.py which is supposed to extract the list > from sources. But for JavaScrip it has hardcoded list of metrics: > https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/acti... > > I think you want to add the new metrics into that list. Thanks, added. I still don't see the actions metrics show up in chrome://histograms/, but that's probably because only histograms show up there?
kelvinp@chromium.org changed reviewers: + kelvinp@chromium.org
https://codereview.chromium.org/1305453002/diff/80001/remoting/webapp/base/js... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/80001/remoting/webapp/base/js... remoting/webapp/base/js/client_session.js:584: chrome.metricsPrivate.recordUserAction( I think this will break our unit tests, as chrome.metricsPrivate won't be defined. Please add the definition to chrome_mocks.js Can we group the all the metrics related call into a separate function like updateMetrics() and call it in setState_() after notifyStateChanges()?
https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2098: </action> These don't seem like user actions. Have you considered using an enumerated histogram instead? https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3820: +<histogram name="Chromoting.Connections.DurationsMinutes.Failed" I'm not sure what the value of separate "DurationsMinutes" and "TimeMs" names is -- I think it would be clearer to just have "Duration" or "Time" histograms, and use the units attribute to distinguish them. That way all duration histograms would be grouped together. https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3827: +<histogram name="Chromoting.Connections.Failed"> I'd recommend including this metric in the enumerated histogram that I recommended for the actions. https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8745: +</histogram> Why is this part of this CL? Seems like there are a bunch of spurious diffs.
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Thanks Ilya and Kelvin. PTAL. https://codereview.chromium.org/1305453002/diff/80001/remoting/webapp/base/js... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/80001/remoting/webapp/base/js... remoting/webapp/base/js/client_session.js:584: chrome.metricsPrivate.recordUserAction( On 2015/08/20 23:21:23, kelvinp wrote: > I think this will break our unit tests, as chrome.metricsPrivate won't be > defined. Please add the definition to chrome_mocks.js > > Can we group the all the metrics related call into a separate function like > updateMetrics() and call it in setState_() after notifyStateChanges()? I ran ./remoting/tools/run_webapp_unittests.py and all tests passed. Is there any other tests I should run? https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/actions/a... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/actions/a... tools/metrics/actions/actions.xml:2098: </action> On 2015/08/20 23:42:46, Ilya Sherman wrote: > These don't seem like user actions. Have you considered using an enumerated > histogram instead? While these are not user actions, they are events experienced by users, so went with user-actions. Would enumerated histograms allow us to do side-by-side comparisons? I notice that on the UMA dashboard, only the sum is shown for enumerated histograms. See https://uma.googleplex.com/p/chrome/histograms/?dayCount=7&endDate=08-19-2015... for example. We actually want to compare each of these metrics across versions. https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3820: +<histogram name="Chromoting.Connections.DurationsMinutes.Failed" On 2015/08/20 23:42:46, Ilya Sherman wrote: > I'm not sure what the value of separate "DurationsMinutes" and "TimeMs" names is > -- I think it would be clearer to just have "Duration" or "Time" histograms, and > use the units attribute to distinguish them. That way all duration histograms > would be grouped together. Yes, I actually first had these simpler, then thought that having the units in the name would make it easier, especially because the units are different for connection times and connecition durations. Now I'm inclined to agree with you again. :-) So changed. https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:3827: +<histogram name="Chromoting.Connections.Failed"> On 2015/08/20 23:42:46, Ilya Sherman wrote: > I'd recommend including this metric in the enumerated histogram that I > recommended for the actions. Moved to actions.xml for now. https://codereview.chromium.org/1305453002/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:8745: +</histogram> On 2015/08/20 23:42:46, Ilya Sherman wrote: > Why is this part of this CL? Seems like there are a bunch of spurious diffs. Not sure what happened here. Fixed.
Anand and I discussed out of band, and agreed that an enumerated histogram is probably better for this data after all.
Thanks Ilya. Please take a look at the latest patch-set. https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:587: buckets: 7 We only want to log when the connection state is after Initializing. So min is 2. The max expected value is 6, and histograms should be defined with the max to be 1 above the expected value. And # of buckets should 1 above the total distinct values to account for overflow. I'll add these comments to the code later. Just sending this out for review now. LMK if any of these are incorrect. https://codereview.chromium.org/1305453002/diff/150001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1305453002/diff/150001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:71077: + <int value="2" label="Connecting"/> Guessing it is OK to not have an enum of value 0?
I think you still need to update chrome mocks. https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:569: if (this.state_ > 1) { Currently initializing doesn't fire so this check is not necessary. And if we ended up firing initializing one day, don't we want to upload that to UMA as well? https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:585: min: 2, Use enum values from remoting.ClientSession.State https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:586: max: 7, The enum range is not consecutive, for example CONNECTION_CANCELED is -3 and FAILED is 6
https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:86: const int kConnectionDurationHistogramBuckets = 50; Not sure if 50 histograms is enough. Are these distributed linearly? I would guess most sessions are shorter than 1 hour, but with 50 buckets over 24 hours we will have only about 2 for the first hour https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:432: break; nit: Add NOTREACHED() here and move to the beginning of the list. https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.h:291: // Time when a connection request was started. These comments do not add anything. Remove?
Patchset #5 (id:170001) has been deleted
Thanks Sergey & Kelvin. PTAL at latest patch-set. https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:86: const int kConnectionDurationHistogramBuckets = 50; On 2015/08/21 18:26:48, Sergey Ulanov wrote: > Not sure if 50 histograms is enough. Are these distributed linearly? I would > guess most sessions are shorter than 1 hour, but with 50 buckets over 24 hours > we will have only about 2 for the first hour These are log-scaled. Added a note about that in the comments. https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.cc:432: break; On 2015/08/21 18:26:48, Sergey Ulanov wrote: > nit: Add NOTREACHED() here and move to the beginning of the list. Done. https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/client/plugin... remoting/client/plugin/chromoting_instance.h:291: // Time when a connection request was started. On 2015/08/21 18:26:48, Sergey Ulanov wrote: > These comments do not add anything. Remove? Done. https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:569: if (this.state_ > 1) { On 2015/08/21 16:29:23, kelvinp wrote: > Currently initializing doesn't fire so this check is not necessary. > And if we ended up firing initializing one day, don't we want to upload that to > UMA as well? Done. https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:585: min: 2, On 2015/08/21 16:29:24, kelvinp wrote: > Use enum values from remoting.ClientSession.State Done. https://codereview.chromium.org/1305453002/diff/150001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:586: max: 7, On 2015/08/21 16:29:23, kelvinp wrote: > The enum range is not consecutive, for example CONNECTION_CANCELED is -3 and > FAILED is 6 Thanks. PTAL at the latest implementation. I considered what you mentioned offline about checking for the value of each state and calling the corresponding enum for recordValue. However, this does not fix the problem of keeping the 2 lists, the one here and the other in histograms.xml in sync. So going with what seems to be a cleaner implementation.
https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:143: // Chromoting.Connections histogram. The 2 lists should be kept in sync. Please also document that the enum should be treated as append-only for this reason. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:588: min: remoting.ClientSession.State.MIN_STATE_ENUM, Hmm, I'm not actually sure whether negative buckets work smoothly for histograms. Could you please check that chrome://histograms shows what you'd expect? If it doesn't, an easy tweak is to simply add 3 (or, equivalently, subtract MIN_STATE_ENUM) to the logged values. (Though, this tweak does mean that you'd need to freeze MIN_STATE_ENUM at -3 for all time; otherwise, the semantics of the recorded enum values would change each time that MIN_STATE_ENUM changes.) https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:593: (remoting.ClientSession.State.MIN_STATE_ENUM) + 2 MAX_STATE_ENUM Is already reserving an extra bucket, so I'm not sure why this is +2 rather than +1. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:593: (remoting.ClientSession.State.MIN_STATE_ENUM) + 2 nit: Is the indentation correct here? https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3813: + <summary>The states that Chromoting connections go through.</summary> Please document when this is recorded -- each time that the connection enters a new state? https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3836: + The time taken to authenticate as part of a Chromoting conneciton request. nit: typo: conneciton
Patchset #6 (id:210001) has been deleted
Patchset #6 (id:230001) has been deleted
Patchset #6 (id:250001) has been deleted
Patchset #6 (id:260001) has been deleted
Thanks Ilya. PTAL. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:143: // Chromoting.Connections histogram. The 2 lists should be kept in sync. On 2015/08/21 19:48:29, Ilya Sherman wrote: > Please also document that the enum should be treated as append-only for this > reason. Done. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:588: min: remoting.ClientSession.State.MIN_STATE_ENUM, On 2015/08/21 19:48:29, Ilya Sherman wrote: > Hmm, I'm not actually sure whether negative buckets work smoothly for > histograms. Could you please check that chrome://histograms shows what you'd > expect? If it doesn't, an easy tweak is to simply add 3 (or, equivalently, > subtract MIN_STATE_ENUM) to the logged values. (Though, this tweak does mean > that you'd need to freeze MIN_STATE_ENUM at -3 for all time; otherwise, the > semantics of the recorded enum values would change each time that MIN_STATE_ENUM > changes.) Thanks. Negative values were actually not working in chrome://histograms. Updated to use only non-negative enum values. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:593: (remoting.ClientSession.State.MIN_STATE_ENUM) + 2 On 2015/08/21 19:48:29, Ilya Sherman wrote: > nit: Is the indentation correct here? Done. https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:593: (remoting.ClientSession.State.MIN_STATE_ENUM) + 2 On 2015/08/21 19:48:29, Ilya Sherman wrote: > MAX_STATE_ENUM Is already reserving an extra bucket, so I'm not sure why this is > +2 rather than +1. Thanks for catching. Forgot to update here when changing the value in the SessionState enum. https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3813: + <summary>The states that Chromoting connections go through.</summary> On 2015/08/21 19:48:29, Ilya Sherman wrote: > Please document when this is recorded -- each time that the connection enters a > new state? Done. https://codereview.chromium.org/1305453002/diff/190001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:3836: + The time taken to authenticate as part of a Chromoting conneciton request. On 2015/08/21 19:48:29, Ilya Sherman wrote: > nit: typo: conneciton Thanks. Fixed.
https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/js_pro... File remoting/webapp/js_proto/chrome_mocks.js (right): https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/js_pro... remoting/webapp/js_proto/chrome_mocks.js:251: }; Nit: put }; on the same line with { Same below https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:600: chrome.metricsPrivate.recordValue(metricDescription, state - Consider state == MIN_STATE_ENUM, the difference would be 0, which is smaller than the min of 1 defined above.
Metrics LGTM % the remaining comments. If you wanted to follow up with a CL that adds a new method to the metricsPrivate API specifically for logging enumerated histograms, I'd be very happy to review that CL =) https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:162: HISTOGRAM_MAX: 10, // According to src/base/metrics/histogram.h, for a UMA Optional nit: I think it might be clearer to define a constant that's equal to 6 or 7 here (i.e. is obviously related to "FAILED"), and then perform math to compute the maximum for the histogram below. https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:596: // The # of buckets should include 1 for overflow. This comment is misleading -- the +1 below is actually for the underflow bucket, which is the bucket with index 0. The overflow bucket is accounted for by setting max to be 1 more than the maximum value you ever plan to record. (The reason that the underflow bucket is used freely, but the overflow bucket is not, is that this allows the enumeration to be extended in subsequent versions of Chrome.) https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:600: chrome.metricsPrivate.recordValue(metricDescription, state - On 2015/08/21 21:32:09, kelvinp wrote: > Consider state == MIN_STATE_ENUM, the difference would be 0, which is smaller > than the min of 1 defined above. That is actually ok -- enumerated histograms typically do use the 0-indexed bucket, which is normally the underflow bucket, because enumerated histograms don't have any underflow. This is a weird quirk of the histograms API, and would be much easier to reason about if the metricsPrivate API just provided a method for recording enumerated histograms.
Patchset #7 (id:300001) has been deleted
Thanks for the quick turnaround, Ilya and Kelvin. PS #7 addresses your latest comments. Re. adding more methods to metricsPrivate, we'd also like to have them included in uma_private, for use in plugins. I'll open a tracking bug. :-) https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/js_pro... File remoting/webapp/js_proto/chrome_mocks.js (right): https://codereview.chromium.org/1305453002/diff/190001/remoting/webapp/js_pro... remoting/webapp/js_proto/chrome_mocks.js:251: }; On 2015/08/21 21:32:09, kelvinp wrote: > Nit: put }; on the same line with { > Same below Done. https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:162: HISTOGRAM_MAX: 10, // According to src/base/metrics/histogram.h, for a UMA On 2015/08/21 21:39:48, Ilya Sherman wrote: > Optional nit: I think it might be clearer to define a constant that's equal to 6 > or 7 here (i.e. is obviously related to "FAILED"), and then perform math to > compute the maximum for the histogram below. Done. https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:596: // The # of buckets should include 1 for overflow. On 2015/08/21 21:39:48, Ilya Sherman wrote: > This comment is misleading -- the +1 below is actually for the underflow bucket, > which is the bucket with index 0. The overflow bucket is accounted for by > setting max to be 1 more than the maximum value you ever plan to record. (The > reason that the underflow bucket is used freely, but the overflow bucket is not, > is that this allows the enumeration to be extended in subsequent versions of > Chrome.) Done. https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:600: chrome.metricsPrivate.recordValue(metricDescription, state - On 2015/08/21 21:32:09, kelvinp wrote: > Consider state == MIN_STATE_ENUM, the difference would be 0, which is smaller > than the min of 1 defined above. Added comment explaining how this works. Tested by seeing that count for the Canceled action (and, luckily, not Dropped, which would be harder to test) which maps to MIN_STATE_ENUM gets updated as expected. https://codereview.chromium.org/1305453002/diff/280001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:600: chrome.metricsPrivate.recordValue(metricDescription, state - On 2015/08/21 21:39:48, Ilya Sherman wrote: > On 2015/08/21 21:32:09, kelvinp wrote: > > Consider state == MIN_STATE_ENUM, the difference would be 0, which is smaller > > than the min of 1 defined above. > > That is actually ok -- enumerated histograms typically do use the 0-indexed > bucket, which is normally the underflow bucket, because enumerated histograms > don't have any underflow. This is a weird quirk of the histograms API, and > would be much easier to reason about if the metricsPrivate API just provided a > method for recording enumerated histograms. Acknowledged.
lgtm
lgtm
The CQ bit was checked by anandc@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/1305453002/#ps320001 (title: "Include min and max enum in SessionState enum definition.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305453002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305453002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by anandc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org, isherman@chromium.org, kelvinp@chromium.org Link to the patchset: https://codereview.chromium.org/1305453002/#ps340001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1305453002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1305453002/340001
Message was sent while issue was closed.
Committed patchset #8 (id:340001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/49b2fcd7c800c94961c5644b847639dabccdc7e7 Cr-Commit-Position: refs/heads/master@{#344930}
Message was sent while issue was closed.
https://codereview.chromium.org/1305453002/diff/340001/remoting/webapp/base/j... File remoting/webapp/base/js/client_session.js (right): https://codereview.chromium.org/1305453002/diff/340001/remoting/webapp/base/j... remoting/webapp/base/js/client_session.js:570: remoting.ClientSession.State.MIN_STATE_ENUM); I'm pretty sure you're missing a "+1" in this computation :( |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
