Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
311133006:
Add more Rappor metrics (Closed)
|
Created:
6 years, 6 months ago by Steven Holte Modified:
6 years, 6 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, Ilya Sherman, jar+watch_chromium.org, odean, ulfar, vpihur Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd more Rappor metrics
Adds Rappor metrics for Default Search Engine, Default Startup Tabs, and
New Tab Pages.
BUG=381325
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276357
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 6
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Remove NTP metric #
Total comments: 6
Patch Set 9 : #Patch Set 10 : Rebase #
Messages
Total messages: 29 (0 generated)
You should create a crbug to track this change. https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... chrome/browser/prefs/pref_metrics_service.cc:115: rappor::SampleETLDp1(g_browser_process->rappor_service(), Is this doing really what you intend? It looks like this will be logged multiple times, as it's iterating over multiple URLs. Additionally, it looks to be done only in the "restore on start up based on prefs" case, which I'm not sure the meaning of. I guess an owner of this code should review it, since I don't have enough understanding of it. https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... chrome/browser/prefs/pref_metrics_service.cc:116: "Settings.StartupPage", Not in this CL, but now that we're getting a non-trivial number of metrics, we should add a rappor.xml file to list them, describe them and specify owners. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.cc:15: if (!service) return; Nit: Put return on next line. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.h:16: // Record the ETLD+1 of a url to a Rappor metric. Nit: Mention that service may be NULL and what happens in that case. Same below. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.h:22: void SampleETLDp1(RapporService* service, Don't overload function names. Use a different name for this one.
https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... chrome/browser/prefs/pref_metrics_service.cc:115: rappor::SampleETLDp1(g_browser_process->rappor_service(), My assumption is that this is when chrome://settings/ "On startup" is set to "Open a specific page or set of pages", and we'd collect a sample for each page, though we'll only generate one report, with more bits set. On 2014/06/05 14:23:35, Alexei Svitkine wrote: > Is this doing really what you intend? > > It looks like this will be logged multiple times, as it's iterating over > multiple URLs. > > Additionally, it looks to be done only in the "restore on start up based on > prefs" case, which I'm not sure the meaning of. I guess an owner of this code > should review it, since I don't have enough understanding of it. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.cc:15: if (!service) return; On 2014/06/05 14:23:35, Alexei Svitkine wrote: > Nit: Put return on next line. Done. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.h:16: // Record the ETLD+1 of a url to a Rappor metric. On 2014/06/05 14:23:35, Alexei Svitkine wrote: > Nit: Mention that service may be NULL and what happens in that case. Same below. Done. https://codereview.chromium.org/311133006/diff/1/components/rappor/rappor_uti... components/rappor/rappor_utils.h:22: void SampleETLDp1(RapporService* service, On 2014/06/05 14:23:35, Alexei Svitkine wrote: > Don't overload function names. Use a different name for this one. Done.
LGTM https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/1/chrome/browser/prefs/pref_me... chrome/browser/prefs/pref_metrics_service.cc:115: rappor::SampleETLDp1(g_browser_process->rappor_service(), On 2014/06/05 20:17:07, Steven Holte wrote: > My assumption is that this is when chrome://settings/ "On startup" is set to > "Open a specific page or set of pages", and we'd collect a sample for each page, > though we'll only generate one report, with more bits set. Ah, that makes sense. Please verify your assumption with an owner, though.
+battre for pref_metrics_service.cc, particularly confirming that the conditions for sampling startup pages are correct. +pkasting for template_url_service.cc
This change has no tests. https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.cc:18: rappor::ETLD_PLUS_ONE_RAPPOR_TYPE, Nit: All lines of args must be indented the same. (4 places) https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.h:18: void SampleETLDp1FromGURL(RapporService* service, Please don't abbreviate "plus 1" like this in function names. If you want to be consistent with the registry-controlled domain service code, use "SampleDomainAndRegistryFromHost". "eTLD+1" isn't a phrase we use in the codebase. How come these aren't public static member functions of RapporService? That seems like a better place for them; then callers are shielded from knowing that you're currently instantiating this service as a global on the browser process (which will help you if this someday turns into a profile keyed service or something). https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.h:19: const std::string& metric, Nit: Indenting (multiple places)
Added a test that calling the util functions does not cause a crash, but I don't think there is any real testable behavior change introduced in this CL. https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.cc:18: rappor::ETLD_PLUS_ONE_RAPPOR_TYPE, On 2014/06/05 23:37:14, Peter Kasting wrote: > Nit: All lines of args must be indented the same. (4 places) Done. https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... File components/rappor/rappor_utils.h (right): https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.h:18: void SampleETLDp1FromGURL(RapporService* service, On 2014/06/05 23:37:14, Peter Kasting wrote: > Please don't abbreviate "plus 1" like this in function names. If you want to be > consistent with the registry-controlled domain service code, use > "SampleDomainAndRegistryFromHost". "eTLD+1" isn't a phrase we use in the > codebase. Done. > > How come these aren't public static member functions of RapporService? That > seems like a better place for them; then callers are shielded from knowing that > you're currently instantiating this service as a global on the browser process > (which will help you if this someday turns into a profile keyed service or > something). Not sure I quite understand this suggestion. Making a static member function would effectively just be renaming it to RapporService::SampleDomainAndRegistryFromGURL. Are you suggesting also removing the first member argument and making it take g_browser_process->rappor_service() internally? If so, the problem is that since this code is in components/, it doesn't know that g_browser_process exists, or what a browser process is. https://codereview.chromium.org/311133006/diff/20001/components/rappor/rappor... components/rappor/rappor_utils.h:19: const std::string& metric, On 2014/06/05 23:37:14, Peter Kasting wrote: > Nit: Indenting (multiple places) Done.
My thought with the tests was that you could test that passing in different URLs and hostnames actually caused a sample to be captured for the correct hostname -- in other words, more of an end-to-end test. As for naming/location of these functions, it seems like there are two options, either one of which would be an improvement on the current situation. You could leave these fucntions unchanged and simply move them to be inside RapporService, which would have the advantage of keeping the functionality localized to one class instead of adding _utils files (despite the number of these in the codebase, we try not to add more). The other would be to move these to the Chrome side, say chrome/browser/rappor_utils.*, so you can go ahead and internalize the use of g_browser_process into them, simplifying the callers. Up to you. LGTM once you decide what you think an appropriate response to the above comments is. https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... File components/rappor/rappor_utils_unittest.cc (right): https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... components/rappor/rappor_utils_unittest.cc:15: SampleDomainAndRegistryFromHost(NULL, "", ""); Nit: Use std::string() in place of "" (many places)
pref_metrics_service.cc: LGTM... Just a couple of things that you should be aware of. https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_metrics_service.cc:95: ntp_url); Please check whether this does what you expect if an extension overrides the NTP. You can try for example https://chrome.google.com/webstore/detail/empty-new-tab-page/dpjamkmjmigaoobj... https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_metrics_service.cc:119: start_url); Just be aware that you get >1 sample per user/profile. Does your pipeline support this? https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... components/rappor/rappor_utils.cc:21: gurl, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)); please be aware that this does not record anything meaningful for chrome-extensions://extensionID/ type of domains.
Renamed rappor_utils to rappor_sampling to avoid it becoming a kitchen sink. End to end testing would be tricky, since the output at the other end is designed to make it hard to know what the input was, and mocking that out would just leave testing of GetDomainAndRegistry. If we expand the logic for extracting information out of the URLs we can add tests for that. Not moving the sampling code to chrome/browser, since this might be useful to some other components/ code that wants to record a sample from a URL. https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_metrics_service.cc:95: ntp_url); On 2014/06/06 09:49:43, battre wrote: > Please check whether this does what you expect if an extension overrides the > NTP. You can try for example > https://chrome.google.com/webstore/detail/empty-new-tab-page/dpjamkmjmigaoobj... It does not. It records whatever would have been displayed. Any idea what I could check to see if the NTP has been overriden? https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... File components/rappor/rappor_utils.cc (right): https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... components/rappor/rappor_utils.cc:21: gurl, net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)); On 2014/06/06 09:49:43, battre wrote: > please be aware that this does not record anything meaningful for > chrome-extensions://extensionID/ type of domains. Noted, though we should still be able to determine a limit on the number of URLs we see of this type. https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... File components/rappor/rappor_utils_unittest.cc (right): https://codereview.chromium.org/311133006/diff/40001/components/rappor/rappor... components/rappor/rappor_utils_unittest.cc:15: SampleDomainAndRegistryFromHost(NULL, "", ""); On 2014/06/06 02:05:48, Peter Kasting wrote: > Nit: Use std::string() in place of "" (many places) Done.
On 2014/06/06 21:00:51, Steven Holte wrote: > Renamed rappor_utils to rappor_sampling to avoid it becoming a kitchen sink. > > Not moving the sampling code to chrome/browser, since this might be useful to > some other components/ code that wants to record a sample from a URL. If it becomes useful at some point, you could move it back. It seems unfortunate to put any more code in components/ than is necessary "on the off chance it might be useful", unless you have some actual uses coming up. If you o want this in components/, I don't see why you didn't move these to actually live on the RapporService as I suggested. That seems monotonically better. What do you gain by doing things this way?
On 2014/06/06 21:03:53, Peter Kasting wrote: > On 2014/06/06 21:00:51, Steven Holte wrote: > > Renamed rappor_utils to rappor_sampling to avoid it becoming a kitchen sink. > > > > Not moving the sampling code to chrome/browser, since this might be useful to > > some other components/ code that wants to record a sample from a URL. > > If it becomes useful at some point, you could move it back. It seems > unfortunate to put any more code in components/ than is necessary "on the off > chance it might be useful", unless you have some actual uses coming up. > > If you o want this in components/, I don't see why you didn't move these to > actually live on the RapporService as I suggested. That seems monotonically > better. What do you gain by doing things this way? My main reason for not moving these to RapporService is because the "meat" of what they are doing, canonicalizing samples, and providing a conventional interface for recording them, seems like a separate task from the main job of RapporService, which is recording sets of samples into logs to upload.
On 2014/06/06 21:31:12, Steven Holte wrote: > On 2014/06/06 21:03:53, Peter Kasting wrote: > > On 2014/06/06 21:00:51, Steven Holte wrote: > > > Renamed rappor_utils to rappor_sampling to avoid it becoming a kitchen sink. > > > > > > Not moving the sampling code to chrome/browser, since this might be useful > to > > > some other components/ code that wants to record a sample from a URL. > > > > If it becomes useful at some point, you could move it back. It seems > > unfortunate to put any more code in components/ than is necessary "on the off > > chance it might be useful", unless you have some actual uses coming up. > > > > If you o want this in components/, I don't see why you didn't move these to > > actually live on the RapporService as I suggested. That seems monotonically > > better. What do you gain by doing things this way? > > My main reason for not moving these to RapporService is because the "meat" of > what they are doing, canonicalizing samples, and providing a conventional > interface for recording them, seems like a separate task from the main job of > RapporService, which is recording sets of samples into logs to upload. You don't think that to outside callers, the interface for recording samples and the actual process of recording and uploading samples are basically indistinguishable?
https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... File chrome/browser/prefs/pref_metrics_service.cc (right): https://codereview.chromium.org/311133006/diff/40001/chrome/browser/prefs/pre... chrome/browser/prefs/pref_metrics_service.cc:119: start_url); On 2014/06/06 09:49:43, battre wrote: > Just be aware that you get >1 sample per user/profile. Does your pipeline > support this? I discussed this with Vasyl and Tyler, and it seems like the way we handle multiple samples right now poses some problems for analysis. For now, I've changed this to just record the first startup page, since that's the most significant.
On 2014/06/06 21:35:53, Peter Kasting wrote: > On 2014/06/06 21:31:12, Steven Holte wrote: > > On 2014/06/06 21:03:53, Peter Kasting wrote: > > > On 2014/06/06 21:00:51, Steven Holte wrote: > > > > Renamed rappor_utils to rappor_sampling to avoid it becoming a kitchen > sink. > > > > > > > > Not moving the sampling code to chrome/browser, since this might be useful > > to > > > > some other components/ code that wants to record a sample from a URL. > > > > > > If it becomes useful at some point, you could move it back. It seems > > > unfortunate to put any more code in components/ than is necessary "on the > off > > > chance it might be useful", unless you have some actual uses coming up. > > > > > > If you o want this in components/, I don't see why you didn't move these to > > > actually live on the RapporService as I suggested. That seems monotonically > > > better. What do you gain by doing things this way? > > > > My main reason for not moving these to RapporService is because the "meat" of > > what they are doing, canonicalizing samples, and providing a conventional > > interface for recording them, seems like a separate task from the main job of > > RapporService, which is recording sets of samples into logs to upload. > > You don't think that to outside callers, the interface for recording samples and > the actual process of recording and uploading samples are basically > indistinguishable? Like I said, I don't think renaming rappor::SampleXYZ to rappor::RapporService::SampleXYZ is really changing the external interface significantly, so how the files are structured is driven by internal structure. But I will buy the argument that hiding g_browser_process->rappor_service() from callers is reason to have some code in chrome/browser/, and that parts we might want to reuse in components could be extracted later, so I've moved the code to chrome/browser/rappor/sampling.h/cc
Removed the NTP metric for a seperate change, since I still need to figure out how to determine when that should be recorded.
LGTM https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... File chrome/browser/rappor/sampling.h (right): https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling.h:14: // Record the ETLD+1 of a url to a Rappor metric. Nit: Record -> Records (2 places) I would probably say "domain and registry" instead of "ETLD+1" to match the function name and the registry-controlled domain service terminology. If you think this is super-unclear, you could perhaps add a parenthetical "(a.k.a. the eTLD+1, "foo.co.uk")" or refer the reader to the RCDS comments for a definition. https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling.h:15: // If the service is NULL, this becomes a no-op. Tiny nit: Minor, but I think this might be a little clearer: "If the Rappor service is NULL, this call does nothing." Maybe say when that service might be NULL if any callers would actually care about such a thing. https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... File chrome/browser/rappor/sampling_unittest.cc (right): https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling_unittest.cc:12: // Make sure recording a sample doesn't cause a crash. Nit: Did you mean "recording a sample from an empty host/GURL"? (As it stands the reader wonders why just "recording a sample" might risk crashing. Or is the Rappor service NULL in tests? If so maybe refer to recording a sample with a NULL service?)
I think chrome/browser/rappor should be chrome/browser/metrics/rappor instead.
Moved to chrome/browser/metrics/rappor https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... File chrome/browser/rappor/sampling.h (right): https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling.h:14: // Record the ETLD+1 of a url to a Rappor metric. On 2014/06/09 00:51:14, Peter Kasting wrote: > Nit: Record -> Records (2 places) > > I would probably say "domain and registry" instead of "ETLD+1" to match the > function name and the registry-controlled domain service terminology. If you > think this is super-unclear, you could perhaps add a parenthetical "(a.k.a. the > eTLD+1, "foo.co.uk")" or refer the reader to the RCDS comments for a definition. Done. https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling.h:15: // If the service is NULL, this becomes a no-op. On 2014/06/09 00:51:14, Peter Kasting wrote: > Tiny nit: Minor, but I think this might be a little clearer: "If the Rappor > service is NULL, this call does nothing." Maybe say when that service might be > NULL if any callers would actually care about such a thing. Done. https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... File chrome/browser/rappor/sampling_unittest.cc (right): https://codereview.chromium.org/311133006/diff/140001/chrome/browser/rappor/s... chrome/browser/rappor/sampling_unittest.cc:12: // Make sure recording a sample doesn't cause a crash. On 2014/06/09 00:51:14, Peter Kasting wrote: > Nit: Did you mean "recording a sample from an empty host/GURL"? (As it stands > the reader wonders why just "recording a sample" might risk crashing. Or is the > Rappor service NULL in tests? If so maybe refer to recording a sample with a > NULL service?) Done.
The CQ bit was checked by holte@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/holte@chromium.org/311133006/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/19...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by holte@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/holte@chromium.org/311133006/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
Message was sent while issue was closed.
Change committed as 276357 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
