Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
387903003:
Add metrics for tracking UX impact of non-static CLD data providers (Closed)
|
Created:
6 years, 5 months ago by Andrew Hayden (chromium.org) Modified:
6 years, 5 months ago Reviewers:
simonb1, jar (doing other things), Takashi Toyoshima, asvitkine_google, andrewhayden, simonb (inactive), Alexei Svitkine (slow), Ilya Sherman CC:
chromium-reviews, asvitkine+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd metrics for tracking UX impact of non-static CLD data providers
BUG=367239
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283817
Patch Set 1 #
Total comments: 3
Patch Set 2 : Address comments #Patch Set 3 : Use histos instead of actions #
Total comments: 18
Patch Set 4 : Comments addressed #
Total comments: 2
Patch Set 5 : asvitkine comments #
Messages
Total messages: 23 (0 generated)
Local testing indicates that this works as documented. There are nuances to interpretation on mobile due to things like background tabs, but we need to put the metrics in before we really start experimenting with non-static CLD data providers. PTAL.
PTAL. https://codereview.chromium.org/387903003/diff/1/chrome/renderer/translate/tr... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/1/chrome/renderer/translate/tr... chrome/renderer/translate/translate_helper.cc:89: // that language detection wouldn't have been relevant anywaysm and so a failure "anywaysm" -> "anyways,"
LGTM with noted cosmetic changes. https://codereview.chromium.org/387903003/diff/1/chrome/renderer/translate/tr... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/1/chrome/renderer/translate/tr... chrome/renderer/translate/translate_helper.cc:89: // that language detection wouldn't have been relevant anywaysm and so a failure On 2014/07/14 14:24:13, Andrew Hayden wrote: > "anywaysm" -> "anyways," "anyways" -> "anyway" :-) http://www.dailywritingtips.com/anyway-any-way-or-anyways/ https://codereview.chromium.org/387903003/diff/1/tools/metrics/actions/action... File tools/metrics/actions/actions.xml (right): https://codereview.chromium.org/387903003/diff/1/tools/metrics/actions/action... tools/metrics/actions/actions.xml:10903: deferred as above in (2), but before the user navigated to a different page. Strike 'as above in (2)'.
Is there a reason you're using User Actions here instead of an enumerated histogram with 3 events? It seems that the things you're recording aren't related to user actions, but rather record some internal state about when the CLD data becomes available. An enumerated histogram is a better fit for that, unless I'm missing something.
On 2014/07/14 16:35:52, Alexei Svitkine wrote: > Is there a reason you're using User Actions here instead of an enumerated > histogram with 3 events? Yes. We can't get the histograms out of the renderer process reliably on Android. The behavior in Android is to simply kill renderer processes without a clean shutdown, which means that the timer-based IPC-the-histos-to-the-browser may never happen. Using counters avoids the problem. The alternatives are all ugly, and involve either forcing additional histogram syncs manually or adding some kind of timer to send them at some frequency, which might end up dropping lots of them on the floor. Using simple actions means we get an IPC message sent immediately in all cases, and the math on the results isn't hard. > It seems that the things you're recording aren't related to user actions, but > rather record some internal state about when the CLD data becomes available. An > enumerated histogram is a better fit for that, unless I'm missing something. See above. We can do histograms, but the plumbing is ugly and will lose more data.
I've addressed the comments by adding comments (ha) and fixing grammar. PTAL.
Are you aware that the dashboards we have for user actions are much more limited than the ones for histograms? For example, we don't support advanced filtering (e.g. by OS flavor, country, etc) nor do we support actions on the Variations dashboard. Are you OK with those limitations? IMHO, it would be better to fix the Android histograms problem than to have everyone do hacks like these. Since this ends up sending an IPC to the browser process anyway to record the action, perhaps we can do the same for a histogram? Introduce a method on the RendererThread that simply IPCs a given histogram call directly to the browser. Then, the behavior should be equivalent to what you're achieving with actions in this CL (i.e. you won't suffer from the Android renderer kill issue), yet you'll get all the benefits of using histograms (mentioned above). +Ilya for his thoughts here too.
On 2014/07/15 20:04:57, Alexei Svitkine wrote: > Are you aware that the dashboards we have for user actions are much more limited > than the ones for histograms? > > For example, we don't support advanced filtering (e.g. by OS flavor, country, > etc) nor do we support actions on the Variations dashboard. Are you OK with > those limitations? > > IMHO, it would be better to fix the Android histograms problem than to have > everyone do hacks like these. > > Since this ends up sending an IPC to the browser process anyway to record the > action, perhaps we can do the same for a histogram? Introduce a method on the > RendererThread that simply IPCs a given histogram call directly to the browser. > Then, the behavior should be equivalent to what you're achieving with actions in > this CL (i.e. you won't suffer from the Android renderer kill issue), yet you'll > get all the benefits of using histograms (mentioned above). > > +Ilya for his thoughts here too. +1. If you're willing to pay the IPC cost for each emit of your metric as an action, then presumably you're willing to pay it for an emit of your metric as a histogram as well.
I've uploaded a patchset using histograms, copying the workaround/hack/[dujour] from page_load_histograms.cc to force an IPC call with every recording of this metric. I've verified that this works for both the static and standalone CLD data sources on my workstation. I'm all for addressing the bigger issue here, but this particular task (see the related bug) has already gone on for 6 months and has contributed a healthy amount of code cleanup to Chromium. Happy to file a crbug, happy to help fix it, but the bigger issue is well beyond the scope of this commit. PTAL.
Thanks, this looks much better. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:671: void TranslateHelper::RecordLanguageDetectionTiming(LanguageDetectionTiming Nit: Wrap after ( instead and indent next line 4. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:681: VLOG(1) << "Language detection timing: " << timing; Do you need the VLOG(1) message or is this left over from your own debugging? In general, log messages are discouraged (other than debug variants that are compiled out of release builds). Plus, since you're recording the histogram anyway, you can just see the status in chrome://histograms. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:120: kLanguageDetectionTimingMax // bounding value for this enum Nit: Use same naming convention as for the other enum members.
https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:156: if (deferred_page_seq_no_ == -1) { nit: Although I'm not usually a big fan of the ternary ?: operators, this code seems to cry out to use it: RecordLanguageDetectionTiming((deferred_page_seq_no_ == -1) ? ON_TIME : RESUMED); If you really like it as is, OK... but IMO, it is a lot less reading for a person looking to understand the code. Your comments could be a block comment in front of this line. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:684: content::RenderThread::Get()->UpdateHistograms( How often (max) could this be called? Is it really infrequent, or could it be several times per page? This is where I was pondering aggregation. As you noted, this call will post a task... so there is some chance that it will be delayed... and maybe never get to effectively run. Perhaps at a minimum, we should look into aggregation inside UpadetHistograms(). https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:177: // Record the timing of language detection, immediately sending an IPC-based nit: perhaps better would be to aggregate these requests, so that you can rate limit how often this fires. For instance, you could rate limit by posting a delayed task, and having a bool that indicates that there is already a task pending. Without this, you'll be doing a lot of work (scanning all histograms in the renderer!) each time you do a minor update in your histogram :-(. It might even be interesting to put such functionality into the histogram system itself, in case other folks want to fire such updates "RSN".
once jar's comments are addressed, lgtm with one suggestion. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:622: Having comments on implementation details in source files is nice. But this looks a kind of information for developers who are interested in using this class. So, I prefer having following comments in the header. WDYT?
I'll post a new patchset shortly addressing these comments. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:156: if (deferred_page_seq_no_ == -1) { On 2014/07/16 16:44:44, jar wrote: > nit: Although I'm not usually a big fan of the ternary ?: operators, this code > seems to cry out to use it: > > RecordLanguageDetectionTiming((deferred_page_seq_no_ == -1) ? > ON_TIME : RESUMED); > > If you really like it as is, OK... but IMO, it is a lot less reading for a > person looking to understand the code. Your comments could be a block comment > in front of this line. Ha. OK, will do :) https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:671: void TranslateHelper::RecordLanguageDetectionTiming(LanguageDetectionTiming On 2014/07/16 14:50:40, Alexei Svitkine wrote: > Nit: Wrap after ( instead and indent next line 4. Will do. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:681: VLOG(1) << "Language detection timing: " << timing; On 2014/07/16 14:50:40, Alexei Svitkine wrote: > Do you need the VLOG(1) message or is this left over from your own debugging? > > In general, log messages are discouraged (other than debug variants that are > compiled out of release builds). Plus, since you're recording the histogram > anyway, you can just see the status in chrome://histograms. I put this here so that I don't have to scan through the ENORMOUS page at chrome://histograms to find my data, and also can be sure that it was actually invoked even if the IPC machinery somehow fails. My understanding of VLOG is that it is designed to overcome these problems that make logging bad. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:684: content::RenderThread::Get()->UpdateHistograms( On 2014/07/16 16:44:44, jar wrote: > How often (max) could this be called? Is it really infrequent, or could it be > several times per page? This is where I was pondering aggregation. As you > noted, this call will post a task... so there is some chance that it will be > delayed... and maybe never get to effectively run. > > Perhaps at a minimum, we should look into aggregation inside UpadetHistograms(). > As above, in the great majority of cases this should get called once. Twice should be a very rare occurrence, I estimate less than 1% of the time. Of course, we won't know for sure till we turn it on. The max number of times this would be called per page load is 2. As stated above, I'm happy to file a crbug and contribute to the long-term fix, but adding aggregation is way out of scope for this task. I'm up for helping out, but I think we should do it in a separate change. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:120: kLanguageDetectionTimingMax // bounding value for this enum On 2014/07/16 14:50:40, Alexei Svitkine wrote: > Nit: Use same naming convention as for the other enum members. This is copied from the same place the rest of this approach came from, but I have no qualms about changing it. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:177: // Record the timing of language detection, immediately sending an IPC-based On 2014/07/16 16:44:44, jar wrote: > nit: perhaps better would be to aggregate these requests, so that you can rate > limit how often this fires. > > For instance, you could rate limit by posting a delayed task, and having a bool > that indicates that there is already a task pending. > > Without this, you'll be doing a lot of work (scanning all histograms in the > renderer!) each time you do a minor update in your histogram :-(. > > It might even be interesting to put such functionality into the histogram system > itself, in case other folks want to fire such updates "RSN". Totally agree, but again very far out of scope for what should be a trivial addition to the code here. I don't expect this to fire more than once per page load in the VAST (>99%) majority of use cases.
On 2014/07/17 02:09:31, Takashi Toyoshima (chromium) wrote: > once jar's comments are addressed, lgtm with one suggestion. > > https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... > File chrome/renderer/translate/translate_helper.cc (right): > > https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... > chrome/renderer/translate/translate_helper.cc:622: > Having comments on implementation details in source files is nice. But this > looks a kind of information for developers who are interested in using this > class. So, I prefer having following comments in the header. WDYT? Fair enough. Maybe I'll move them to our Chromium documentation about the data source, instead, and put a link to them in the header. How about that? I could put them in a new section here: http://www.chromium.org/developers/how-tos/compact-language-detector-cld-data...
https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:681: VLOG(1) << "Language detection timing: " << timing; On 2014/07/17 08:36:19, andrewhayden wrote: > On 2014/07/16 14:50:40, Alexei Svitkine wrote: > > Do you need the VLOG(1) message or is this left over from your own debugging? > > > > In general, log messages are discouraged (other than debug variants that are > > compiled out of release builds). Plus, since you're recording the histogram > > anyway, you can just see the status in chrome://histograms. > > I put this here so that I don't have to scan through the ENORMOUS page at > chrome://histograms to find my data, and also can be sure that it was actually > invoked even if the IPC machinery somehow fails. My understanding of VLOG is > that it is designed to overcome these problems that make logging bad. You can filter chrome://histograms/ by substring - e.g. chrome://histograms/Translate.LanguageDetectionTiming I don't think VLOG() overcomes the problems. It still bloats the binary. DVLOG does overcome the problems though (since it will only be enabled in debug builds which we don't ship to users).
Addressing comments, PTAL. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.cc (right): https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:156: if (deferred_page_seq_no_ == -1) { On 2014/07/17 08:36:19, andrewhayden wrote: > On 2014/07/16 16:44:44, jar wrote: > > nit: Although I'm not usually a big fan of the ternary ?: operators, this code > > seems to cry out to use it: > > > > RecordLanguageDetectionTiming((deferred_page_seq_no_ == -1) ? > > ON_TIME : RESUMED); > > > > If you really like it as is, OK... but IMO, it is a lot less reading for a > > person looking to understand the code. Your comments could be a block comment > > in front of this line. > > Ha. OK, will do :) Changed my mind on this one, I like the comments. Ternary is just sugar anyways. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:622: On 2014/07/17 02:09:31, Takashi Toyoshima (chromium) wrote: > Having comments on implementation details in source files is nice. But this > looks a kind of information for developers who are interested in using this > class. So, I prefer having following comments in the header. WDYT? I moved it to the header file. I considered kicking it out to the docs but I think it's better here. I will also add to the docs, but the info really does belong with the class. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:681: VLOG(1) << "Language detection timing: " << timing; On 2014/07/17 13:20:25, Alexei Svitkine wrote: > On 2014/07/17 08:36:19, andrewhayden wrote: > > On 2014/07/16 14:50:40, Alexei Svitkine wrote: > > > Do you need the VLOG(1) message or is this left over from your own > debugging? > > > > > > In general, log messages are discouraged (other than debug variants that are > > > compiled out of release builds). Plus, since you're recording the histogram > > > anyway, you can just see the status in chrome://histograms. > > > > I put this here so that I don't have to scan through the ENORMOUS page at > > chrome://histograms to find my data, and also can be sure that it was actually > > invoked even if the IPC machinery somehow fails. My understanding of VLOG is > > that it is designed to overcome these problems that make logging bad. > > You can filter chrome://histograms/ by substring - e.g. > chrome://histograms/Translate.LanguageDetectionTiming > > I don't think VLOG() overcomes the problems. It still bloats the binary. DVLOG > does overcome the problems though (since it will only be enabled in debug builds > which we don't ship to users). DVLOG it is, then. https://codereview.chromium.org/387903003/diff/40001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.cc:684: content::RenderThread::Get()->UpdateHistograms( On 2014/07/17 08:36:19, andrewhayden wrote: > On 2014/07/16 16:44:44, jar wrote: > > How often (max) could this be called? Is it really infrequent, or could it be > > several times per page? This is where I was pondering aggregation. As you > > noted, this call will post a task... so there is some chance that it will be > > delayed... and maybe never get to effectively run. > > > > Perhaps at a minimum, we should look into aggregation inside > UpadetHistograms(). > > > > As above, in the great majority of cases this should get called once. Twice > should be a very rare occurrence, I estimate less than 1% of the time. Of > course, we won't know for sure till we turn it on. > > The max number of times this would be called per page load is 2. > > As stated above, I'm happy to file a crbug and contribute to the long-term fix, > but adding aggregation is way out of scope for this task. I'm up for helping > out, but I think we should do it in a separate change. Performance comments added here.
lgtm https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... File chrome/renderer/translate/translate_helper.h (right): https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:166: LANGUAGE_DETECTION_TIMING_MAX_VALUE // bounding value for this enum Nit: Use full sentences - capitalized and terminated with a . https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... chrome/renderer/translate/translate_helper.h:226: void RecordLanguageDetectionTiming(LanguageDetectionTiming); Since this is Chromium code (and not blink), please keep the param name in the method signature.
On 2014/07/17 14:39:03, Alexei Svitkine wrote: > lgtm > > https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... > File chrome/renderer/translate/translate_helper.h (right): > > https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... > chrome/renderer/translate/translate_helper.h:166: > LANGUAGE_DETECTION_TIMING_MAX_VALUE // bounding value for this enum > Nit: Use full sentences - capitalized and terminated with a . > > https://codereview.chromium.org/387903003/diff/60001/chrome/renderer/translat... > chrome/renderer/translate/translate_helper.h:226: void > RecordLanguageDetectionTiming(LanguageDetectionTiming); > Since this is Chromium code (and not blink), please keep the param name in the > method signature. Both done. Thanks.
(one minor note, I didn't add periods because it would have pushed the line length of the comments past 80, and I really don't feel that it's worth the annoyance to add the period if I have to add a whole other line and wrap the comment to do it :)
The CQ bit was checked by andrewhayden@google.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/andrewhayden@chromium.org/387903003/80001
Message was sent while issue was closed.
Change committed as 283817 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
