Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
253203002:
Log operator code histogram on new metric log (Closed)
|
Created:
6 years, 7 months ago by bolian Modified:
6 years, 7 months ago CC:
chromium-reviews, jar (doing other things), asvitkine+watch_chromium.org, Matt Welsh Base URL:
https://chromium.googlesource.com/chromium/src.git@api_move Visibility:
Public. |
DescriptionThis is the separate change mentioned in comments of https://codereview.chromium.org/246553003/.
It emits NCN.NetworkOperatorMCCMNC_NewMetricsLog on new metric log creation with the network operator MCC_MNC code
BUG=355604
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269627
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : Added new histogram, removed synthetic trial. #Patch Set 4 : . #Patch Set 5 : . #Patch Set 6 : . #Patch Set 7 : remove unused headers. #Patch Set 8 : . #
Total comments: 20
Patch Set 9 : addressed comments. #Patch Set 10 : sync'ed to head, added test. #
Total comments: 40
Patch Set 11 : addressed more comments. #
Total comments: 20
Patch Set 12 : addressed more comments. #
Total comments: 2
Patch Set 13 : fixed comment nits. #Patch Set 14 : combine and rename metric name. #
Total comments: 17
Patch Set 15 : addressed more comments. #Patch Set 16 : . #Patch Set 17 : create observer in ChromeBrowserMainExtraPartsMetrics::PostBrowserStart() #
Total comments: 6
Patch Set 18 : remove incorrect NotifyOnDidCreateMetricsLog call. #
Total comments: 6
Patch Set 19 : sync'ed to head and addressed comments. #Patch Set 20 : sync again and updated tests. #Patch Set 21 : . #Messages
Total messages: 61 (0 generated)
Hello, This CL annotate each metric log upload with a operator code synthetic trial if available. This is a followup change in comments of https://codereview.chromium.org/246553003/ Thanks, Bolian
Hmm, the synthetic trials framework wasn't really meant for cases like these. It's really meant for cases like recording second-stage field trial behaviour (e.g. if there's a field trial to provide a promo where the user opt-in to a feature, the synthetic trial would only record the users who chose to opt-in). Presumably you're doing this because you believe a histogram is not a good enough tool for this. Assuming that's true, can you elaborate what this provides that a histogram doesn't for your use case? My understanding is you guys run your own analysis scripts. If so, what's the difference between looking at a specific repeated histogram entry vs. a specific repeated field trial entry?
Ben will setup a meeting. Let's discuss the operator code related issues through :)
I just left a comment on https://codereview.chromium.org/246553003/ about the histogram solution. Bottom line is that I don't understand the proposal yet. On Thu May 01 2014 at 11:26:21 AM, <bolian@chromium.org> wrote: > Ben will setup a meeting. Let's discuss the operator code related issues > through > :) > > https://codereview.chromium.org/253203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
We need a way to associate an upload with the operator code when the connection type does not change ( https://codereview.chromium.org/246553003/ log the metric on connection type change). We could do this by remembering the type when a new MetricLog is created and check that the type is not changed at the end of the upload. But synthetic trial is doing exactly that, that is why I am using synthetic trial here.
On 2014/05/01 21:35:58, bolian1 wrote: > We need a way to associate an upload with the operator code when the connection > type does not change ( https://codereview.chromium.org/246553003/ log the metric > on connection type change). We could do this by remembering the type when a new > MetricLog is created and check that the type is not changed at the end of the > upload. But synthetic trial is doing exactly that, that is why I am using > synthetic trial here. I would recommend something more like the following: (1) Always emit to the histogram when a new log is created. (2) Emit to the same histogram any time that the network changes. (1) might be tricky to do around the time that Chrome starts up. If so, you could probably compensate by emitting to the histogram as soon as you're able to read the network operator, i.e. when your component/feature starts up. But if you do that, there's some risk that you'd be racing the metrics system startup, and possibly sometimes emit to the histogram twice for the initial log. In short, you'll want to think carefully about implementing something robust to take startup into account :) (Note that (2) implies that the histogram that you're adding in your other CL likely doesn't need to be a separate histogram, since the data would be included in this one as well.)
The synthetic trail does 1) and 2) except that the trial is not logged when the two type seen by 1) and 2) are different. It seems to me that we should use this mechanism.
On 2014/05/01 22:15:50, bolian wrote: > The synthetic trail does 1) and 2) except that the trial is not logged when the > two type seen by 1) and 2) are different. It seems to me that we should use this > mechanism. IMO, logging the values when (1) and (2) are different is a useful feature. Moreover, this is really not what synthetic trials are designed for. We shouldn't abuse the API just because it happens to currently provide the desired behavior. There's no guarantee that it will continue to do so in the future. It would be better to use an API designed with this use case in mind. I think it will be better to design a new API, rather than trying to expand the synthetic field trial API. We can discuss more concrete details in the meeting tomorrow.
On 2014/05/01 22:24:19, Ilya Sherman wrote: > IMO, logging the values when (1) and (2) are different is a useful feature. > Moreover, this is really not what synthetic trials are designed for. We > shouldn't abuse the API just because it happens to currently provide the desired > behavior. There's no guarantee that it will continue to do so in the future. > It would be better to use an API designed with this use case in mind. I think > it will be better to design a new API, rather than trying to expand the > synthetic field trial API. We can discuss more concrete details in the meeting > tomorrow. This CL adds a MetricsLogObserver, which can be used to for 1) and we have https://codereview.chromium.org/246553003/ for the network change case already. So, this CL can be reduced to do only 1). Yes, let's chat more details tomorrow.
Hello, As we discussed, this CL adds an observer to metric log creation and log the operator code for each metric log. I added Lei for chrome/browser/chrome_browser_main.* changes and the added chrome/browser/chrome_browser_metrics_log_observer.*. Thanks, Bolian
https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:563: MetricsService* metrics = browser_process_->metrics_service(); Why did you move this line up? And should the comment move with it? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. Remove (c) https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:25: void ChromeBrowserMetricsLogObserver::OnNewMetricsLog() { Could you avoid the #ifdefs here by subclassing a ChromeBrowserMetricsLogObserverAndroid? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:30: if (type == net::NetworkChangeNotifier::CONNECTION_2G || Are there no other mobile network types? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: mcc_mnc = 0; So 0 means unknown and ethernet and wifi? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.h (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: remove (c) https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.h:5: #ifndef CHROME_BROWSER_CHROME_BROWSER_METRICS_LOG_OBSERVER_H__ Remove trailing _ here and below. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1217: metrics_log_observers_.RemoveObserver(observer); Are the observers guaranteed to outlive the MetricsService? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:2108: if (metrics_service != NULL) { When would this be null?
https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:563: MetricsService* metrics = browser_process_->metrics_service(); Move it so that the metrics service could be created before adding the log observer. The comment is for the lines below. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/06 17:14:39, bengr1 wrote: > Remove (c) Done. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:25: void ChromeBrowserMetricsLogObserver::OnNewMetricsLog() { I think I only need ifdefs in this one method and I have removed them in the constructor and destructor. With that, using ChromeBrowserMetricsLogObserverAndroid does not seem to improve things. What do you think? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:30: if (type == net::NetworkChangeNotifier::CONNECTION_2G || Yes, the three are on the only cellular network, for now. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: mcc_mnc = 0; Yes. Basically, it means non-cellular network, that we don't care for purpose of this histogram. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.h (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/05/06 17:14:39, bengr1 wrote: > nit: remove (c) Done. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.h:5: #ifndef CHROME_BROWSER_CHROME_BROWSER_METRICS_LOG_OBSERVER_H__ On 2014/05/06 17:14:39, bengr1 wrote: > Remove trailing _ here and below. Done. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1217: metrics_log_observers_.RemoveObserver(observer); For the current usage, Yes. If not in the future, observers should remove themselves before die. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:2108: if (metrics_service != NULL) { Just as a safety check before dereferring it. In theory, an observer could call MetricsServiceHelper::AddMetricsLogObserver before the metrics service created. But not for this CL.
Hello Ilya, Alexei, and Lei Could you take a look at this change? Trying to make it for Friday's branching. And commit queue could take one whole day or more. Thanks, Bolian
Change generally looks good. Any chance you can add a metrics service unit test for the observer functionality?
I mostly defer to Ilya (or jar@) for metrics.
Thanks, added test.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); I am not an owner for this code, but modifying chrome_browser_main.cc for this change seems unlikely to be appropriate -- it's too low a level. Is there not some reasonable //net component that this can be dangled off of? (He asks, being largely unfamiliar with the organization of the //net module.) https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); Hmm, I was expecting that you'd emit to a single histogram, e.g. "Network.NetworkOperatorMCCMNC". What's the advantage of separating _NewMetricsLog from _ConnectionChange? https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1082: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); nit: Please use a ThreadChecker rather than referring to the thread by name. This makes both componentization and testing easier. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1962: MetricsLogObserver* observer) { nit: Looks like this can fit on the previous line. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1964: if (metrics_service != NULL) { nit: No need for "!= NULL". https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1966: } nit: No need for curlies. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:113: class MetricsLogObserver { nit: I think "MetricsServiceObserver" is more accurate, given where this class is defined. Also, could you please extract this class out to its own header file? https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:116: virtual void OnNewMetricsLog() = 0; nit: "OnDidCreateMetricsLog()" or something else involving a verb :) https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:379: // Registers/Unregsiters |observer| to receive MetricsLog notifications. nit: "Un" -> "un" https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:626: friend class ::ChromeBrowserMetricsLogObserver; nit: Alphabetically, this should come before line 625 (though the namespaces make it weird). https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:144: observed_++; nit: Pre-increment. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:147: int observed_; nit: Please leave a space after this. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:147: int observed_; nit: Please make the variable private, and add a getter. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:342: } Could you please also add test coverage for the other two places where the observer gets notified?
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); This is not resolved yet. It is here to make sure the observer is added as soon as the metrics service is created. I am open to better suggestions. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); It is more about the accurate of the name, this one is not due to connection change. The alternative could be _ConnectionChangeOrNewMetricLog, but then we have to assume the first one always the NewMetricsLog and later ones are due to network change. Separating them is clearer to me. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1082: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)); On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: Please use a ThreadChecker rather than referring to the thread by name. > This makes both componentization and testing easier. Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1962: MetricsLogObserver* observer) { after renaming, it cannot. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1964: if (metrics_service != NULL) { On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: No need for "!= NULL". Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1966: } On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: No need for curlies. Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:113: class MetricsLogObserver { On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: I think "MetricsServiceObserver" is more accurate, given where this class > is defined. Also, could you please extract this class out to its own header > file? Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:116: virtual void OnNewMetricsLog() = 0; On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: "OnDidCreateMetricsLog()" or something else involving a verb :) Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:379: // Registers/Unregsiters |observer| to receive MetricsLog notifications. On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: "Un" -> "un" Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:626: friend class ::ChromeBrowserMetricsLogObserver; On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: Alphabetically, this should come before line 625 (though the namespaces > make it weird). Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:144: observed_++; On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: Pre-increment. Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:147: int observed_; On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: Please leave a space after this. Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:147: int observed_; On 2014/05/07 01:03:16, Ilya Sherman wrote: > nit: Please make the variable private, and add a getter. Done. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:342: } The other two places require non-trivial amount of plumbing to prepare the data/environment so that they don't crash, seems an overkill for this test.
https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:367: // Registers/unregsiters |observer| to receive MetricsLog notifications. unregisters https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:636: // Registers/Unregsiters |observer| to receive MetricsLog notifications unregisters https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:637: // from metric service. from the metrics service. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:12: // Will be called when a new MetricsLog is created. Will be called -> Called https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:324: TEST_F(MetricsServiceTest, MetricsServiceObserver) { You should also test with multiple observers. https://codereview.chromium.org/253203002/diff/190001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/253203002/diff/190001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10707: + operator when a new metrics log is created. Value zero mean the connection Value zero mean -> A value of zero means
https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:10: class ChromeBrowserMetricsServiceObserver: public MetricsServiceObserver { Nit: Space before : Also, add a short description of this class. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:15: // MetricsServiceObserver; Nit: ";" -> ":" https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:10: class MetricsServiceObserver { Add a comment explaining what this is for.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); On 2014/05/07 02:20:26, bolian wrote: > This is not resolved yet. It is here to make sure the observer is added as soon > as the metrics service is created. I am open to better suggestions. I explored this more. //net still does not look like a good place for this, as metrics service is created and maintained in //chrome/browser, and the observer should be added as soon as it created. The current place is also where other metrics related stuff happened. I keep it here for now. But I am open to suggestions. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:10: class ChromeBrowserMetricsServiceObserver: public MetricsServiceObserver { On 2014/05/07 17:05:41, Alexei Svitkine wrote: > Nit: Space before : > > Also, add a short description of this class. Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:15: // MetricsServiceObserver; . ? https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:367: // Registers/unregsiters |observer| to receive MetricsLog notifications. On 2014/05/07 16:03:58, bengr1 wrote: > unregisters Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:636: // Registers/Unregsiters |observer| to receive MetricsLog notifications On 2014/05/07 16:03:58, bengr1 wrote: > unregisters Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:637: // from metric service. On 2014/05/07 16:03:58, bengr1 wrote: > from the metrics service. Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:10: class MetricsServiceObserver { On 2014/05/07 17:05:41, Alexei Svitkine wrote: > Add a comment explaining what this is for. Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:12: // Will be called when a new MetricsLog is created. On 2014/05/07 16:03:58, bengr1 wrote: > Will be called -> Called Done. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:324: TEST_F(MetricsServiceTest, MetricsServiceObserver) { On 2014/05/07 16:03:58, bengr1 wrote: > You should also test with multiple observers. Done. https://codereview.chromium.org/253203002/diff/190001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/253203002/diff/190001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10707: + operator when a new metrics log is created. Value zero mean the connection On 2014/05/07 16:03:58, bengr1 wrote: > Value zero mean -> A value of zero means Done.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); On 2014/05/07 18:11:44, bolian wrote: > On 2014/05/07 02:20:26, bolian wrote: > > This is not resolved yet. It is here to make sure the observer is added as > soon > > as the metrics service is created. I am open to better suggestions. > > I explored this more. //net still does not look like a good place for this, as > metrics service is created and maintained in //chrome/browser, and the observer > should be added as soon as it created. The current place is also where other > metrics related stuff happened. I keep it here for now. But I am open to > suggestions. I still think this is unlikely to be the correct layer for making this change: (1) First of all, is g_network_change_notifier guaranteed to be instantiated by the time this code is reached? If not, is it a problem to emit UNKNOWN for the connection type for the initial log? (2) Regardless of anything else, the observer itself can probably live in //net, rather than in //chrome/browser. (3) If you definitely want to register the observer as soon as the MetricsService is constructed, the best place to do that is from the MetricsService constructor. That also has the advantage that we can remove the friend declaration. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); On 2014/05/07 02:20:26, bolian wrote: > It is more about the accurate of the name, this one is not due to connection > change. > The alternative could be _ConnectionChangeOrNewMetricLog, but then we have to > assume the first one always the NewMetricsLog and later ones are due to network > change. Separating them is clearer to me. Why does it matter which one is first? If there are multiple changes during a single log period, you wouldn't be able to sequence those anyway. Do you really need the sequencing data? https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:15: // MetricsServiceObserver; On 2014/05/07 18:11:44, bolian wrote: > . ? *Shrug.* The typical style is to use a colon. https://codereview.chromium.org/253203002/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:11: // An observer must be added, removed, and notified in the same thread. nit: "in the same thread" -> "on the same thread". The MetricsService class is not thread safe, and runs on the main thread.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); I keep remind myself that MetricsServiceObserver has nothing to do with //net :). It just happen to record the connection status when notification is received, so why move it to //net. I actually prefer register in MetricsService constructor, but that idea was shot down during the meeting. Should we do that and move everything to //chrome/browser/metrics, I think that still make sense as long as the observer only log the environment info like the operator code. Another place could be BrowserProcessImpl::CreateMetricsService(). What do you think? https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); Mainly, I want to differentiate below two cases: A: no connection change during the log (with only _NewMetricsLog and no _ConnectionChange) B: connection changed during the long (with one or more _ConnectionChanges). In case A, we can trust all the data happened on the same cellular network, in case B, we would take the result with a grain of salt. Combining them makes it not so obvious if even possible. https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/190001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.h:15: // MetricsServiceObserver; On 2014/05/07 23:57:40, Ilya Sherman wrote: > On 2014/05/07 18:11:44, bolian wrote: > > . ? > > *Shrug.* The typical style is to use a colon. Done. https://codereview.chromium.org/253203002/diff/210001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/210001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:11: // An observer must be added, removed, and notified in the same thread. On 2014/05/07 23:57:40, Ilya Sherman wrote: > nit: "in the same thread" -> "on the same thread". The MetricsService class is > not thread safe, and runs on the main thread. Done.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); On 2014/05/08 00:54:51, bolian wrote: > I keep remind myself that MetricsServiceObserver has nothing to do with //net > :). It just happen to record the connection status when notification is > received, so why move it to //net. > > I actually prefer register in MetricsService constructor, but that idea was shot > down during the meeting. Should we do that and move everything to > //chrome/browser/metrics, I think that still make sense as long as the observer > only log the environment info like the operator code. Perhaps there was some miscommunication. I don't recall discussing where the AddObserver() call should take place. Note: I'm suggesting a one-line change to the constructor: just adding the observer. I'm not suggesting getting rid of the observer pattern. The code to emit to "NCN.NetworkOperatorMCCMNC_ConnectionChange" currently lives under //net. Why isn't it appropriate for the observer code to reside there as well? The reason that I prefer to have the observer live under //net is that it's recording //net-level concepts. It's not recording anything to do with Chrome's initialization (other than recording a value at startup), and it's not recording a general-purpose metric. Neither //chrome/browser/chrome_browser_main.cc nor //chrome/browser/metrics should be a dumping ground. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); On 2014/05/08 00:54:51, bolian wrote: > Mainly, I want to differentiate below two cases: > A: no connection change during the log (with only _NewMetricsLog and no > _ConnectionChange) > B: connection changed during the long (with one or more _ConnectionChanges). > > In case A, we can trust all the data happened on the same cellular network, in > case B, we would take the result with a grain of salt. > > Combining them makes it not so obvious if even possible. If you emit to a single histogram, you're in case A if the histogram has just a single sample, and case B if the histogram has more than one sample.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); We have abstracted it to be an observer to MetricsService, MetricsServiceObserver could be used to do other things on new metrics log in the future, not just //net-level things. So, it is not proper to live under //net. We can add observers in constructor, but we still need to put the observers in somewhere, so it does not change things much -- unless we move the observer implementation itself into //chrome/browser/metrics, for this case. Other observers in the future can live elsewhere. That will improve/simply things a lot. What do think? https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); hmm, I can do that (and remove the metric to be _ConnectionChangeOrNewMetricsLog), but I still worry that there might be cases (now or in the future), the current _NewMetricsLog is not logged for some bug and there is one _ConnectionChange logged. That will mess up our analysis. Separating them could avoid that. What is the main benefit of combing them other than less histograms to maintain?
I think if we put it in the constructor now, we'll need to revisit that decision shortly when MetricsService will move to //components, so I'm not a fan of doing that. chrome_browser_main.cc seems fine to me, OTOH since that's where we do initialization of metrics. On Wed, May 7, 2014 at 9:29 PM, <bolian@chromium.org> wrote: > > https://codereview.chromium.org/253203002/diff/170001/ > chrome/browser/chrome_browser_main.cc > File chrome/browser/chrome_browser_main.cc (right): > > https://codereview.chromium.org/253203002/diff/170001/ > chrome/browser/chrome_browser_main.cc#newcode565 > chrome/browser/chrome_browser_main.cc:565: > browser_metrics_log_observer_.reset(new > ChromeBrowserMetricsLogObserver()); > We have abstracted it to be an observer to MetricsService, > MetricsServiceObserver could be used to do other things on new metrics > log in the future, not just //net-level things. So, it is not proper to > live under //net. > > We can add observers in constructor, but we still need to put the > observers in somewhere, so it does not change things much -- unless we > move the observer implementation itself into //chrome/browser/metrics, > for this case. Other observers in the future can live elsewhere. That > will improve/simply things a lot. > > What do think? > > > https://codereview.chromium.org/253203002/diff/170001/ > chrome/browser/chrome_browser_metrics_log_observer.cc > File chrome/browser/chrome_browser_metrics_log_observer.cc (right): > > https://codereview.chromium.org/253203002/diff/170001/ > chrome/browser/chrome_browser_metrics_log_observer.cc#newcode36 > chrome/browser/chrome_browser_metrics_log_observer.cc:36: > "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); > hmm, I can do that (and remove the metric to be > _ConnectionChangeOrNewMetricsLog), but I still worry that there might be > cases (now or in the future), the current _NewMetricsLog is not logged > for some bug and there is one _ConnectionChange logged. That will mess > up our analysis. Separating them could avoid that. What is the main > benefit of combing them other than less histograms to maintain? > > https://codereview.chromium.org/253203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_main.cc:565: browser_metrics_log_observer_.reset(new ChromeBrowserMetricsLogObserver()); On 2014/05/08 01:29:58, bolian wrote: > We have abstracted it to be an observer to MetricsService, > MetricsServiceObserver could be used to do other things on new metrics log in > the future, not just //net-level things. So, it is not proper to live under > //net. > > We can add observers in constructor, but we still need to put the observers in > somewhere, so it does not change things much -- unless we move the observer > implementation itself into //chrome/browser/metrics, for this case. Other > observers in the future can live elsewhere. That will improve/simply things a > lot. > > What do think? Hmm, I think I understand your concern: You don't want //net to depend on //chrome. That's fair. How about //chrome/browser/net, then? https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); On 2014/05/08 01:29:58, bolian wrote: > hmm, I can do that (and remove the metric to be > _ConnectionChangeOrNewMetricsLog), but I still worry that there might be cases > (now or in the future), the current _NewMetricsLog is not logged for some bug > and there is one _ConnectionChange logged. That will mess up our analysis. > Separating them could avoid that. What is the main benefit of combing them other > than less histograms to maintain? The benefit is indeed having fewer histograms to maintain. That's a real and important benefit ;) If there is a bug in your metrics emission code, the analysis is going to run into problems anyway. If you use two separate histograms, and sometimes nothing is emitted to _NewMetricsLog, then sometimes you won't see any data at all for a given upload, which would also mess up your analysis. (Btw, I would recommend dropping the suffix entirely, rather than using "_ConnectionChangeOrNewMetricsLog".)
On 2014/05/08 03:07:27, Alexei Svitkine wrote: > I think if we put it in the constructor now, we'll need to revisit that > decision shortly when MetricsService will move to //components, so I'm not > a fan of doing that. chrome_browser_main.cc seems fine to me, OTOH since > that's where we do initialization of metrics. So, none of us are owners of chrome_browser_main.cc. Maybe it would be better to loop in said OWNERS, rather than debating amongst ourselves :P Note that even after we move the code to //components, there will remain parts of MetricsService that live in //chrome, because there are unavoidable dependencies on //chrome code. That's fine. We can use the common delegate pattern to support that need.
Let loop in Lei for that. Hi Lei, Could you take a look at the change at chrome/browser/chrome_browser_main.cc? I want register a MetricsServiceObserver after the MetricsServiceWe is created. You might want to look back a few comments about the discussion. But to summarize, Alexei and I think the current place is fine as other metrics stuff are init'ed here. Ilya has concern that this is too low level for such a change. https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/170001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:36: "NCN.NetworkOperatorMCCMNC_NewMetricsLog", mcc_mnc); fair enough. I combined them and rename it by removing the suffix.
Thanks. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.cc (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.cc:37: UMA_HISTOGRAM_SPARSE_SLOWLY("NCN.NetworkOperatorMCCMNC", mcc_mnc); Can you share lines 27-37 with the equivalent code in //net, rather than implementing it twice?
Some questions: - Will there be another class that implements MetricsLogObserver in the near future? - When will the metrics componentization happen? Very soon? In 6 months? Who knows? https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:25: void ChromeBrowserMetricsLogObserver::OnNewMetricsLog() { On 2014/05/06 18:00:08, bolian wrote: > I think I only need ifdefs in this one method and I have removed them in the > constructor and destructor. With that, using > ChromeBrowserMetricsLogObserverAndroid does not seem to improve things. What do > you think? Why do you think ChromeBrowserMetricsLogObserverAndroid does not improve things? As is, an empty observer gets called for no reason on non-Android builds. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:299: ResponseStatus ResponseCodeToStatus(int response_code) { Random note to isherman: this looks line dead code. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:593: ObserverList<MetricsServiceObserver> observers_; nit: newline after this https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:594: // Confirms single-threaded access to observers_ in debug builds. nit: |observers_| https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:595: base::ThreadChecker thread_checker_; #include "base/threading/thread_checker.h" https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:595: base::ThreadChecker thread_checker_; It might be good to guard other methods in MetricsService with |thread_checker_| as well, but that can's outside the scope of this CL. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:603: FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, MetricsServiceObserver); nit: alphabetical order, put after IsPluginProcess https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:146: int observed() { return observed_; } nit: const https://codereview.chromium.org/253203002/diff/250001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/253203002/diff/250001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1224: 'browser/metrics/metrics_service_observer.h', nit: alphabetical order. _ob should follow _an
Thanks for the review! - Will there be another class that implements MetricsLogObserver in the near future? NO. - When will the metrics componentization happen? Very soon? In 6 months? Who knows? I leave this to Ilya or Alexie. https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_log_observer.cc (right): https://codereview.chromium.org/253203002/diff/130001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_log_observer.cc:25: void ChromeBrowserMetricsLogObserver::OnNewMetricsLog() { ok. that makes sense. Will address this in the next patchset. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/chrome_b... File chrome/browser/chrome_browser_metrics_service_observer.cc (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/chrome_b... chrome/browser/chrome_browser_metrics_service_observer.cc:37: UMA_HISTOGRAM_SPARSE_SLOWLY("NCN.NetworkOperatorMCCMNC", mcc_mnc); On 2014/05/08 03:48:40, Ilya Sherman wrote: > Can you share lines 27-37 with the equivalent code in //net, rather than > implementing it twice? Done. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.h (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:593: ObserverList<MetricsServiceObserver> observers_; On 2014/05/08 05:40:19, Lei Zhang wrote: > nit: newline after this Done. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:594: // Confirms single-threaded access to observers_ in debug builds. On 2014/05/08 05:40:19, Lei Zhang wrote: > nit: |observers_| Done. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:595: base::ThreadChecker thread_checker_; On 2014/05/08 05:40:19, Lei Zhang wrote: > #include "base/threading/thread_checker.h" Done. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:595: base::ThreadChecker thread_checker_; I will leave this to isherman@ or asvitkine@. Let me if you want me to file a bug to track this. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.h:603: FRIEND_TEST_ALL_PREFIXES(MetricsServiceTest, MetricsServiceObserver); On 2014/05/08 05:40:19, Lei Zhang wrote: > nit: alphabetical order, put after IsPluginProcess Done. https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_unittest.cc (right): https://codereview.chromium.org/253203002/diff/250001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_unittest.cc:146: int observed() { return observed_; } On 2014/05/08 05:40:19, Lei Zhang wrote: > nit: const Done. https://codereview.chromium.org/253203002/diff/250001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/253203002/diff/250001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:1224: 'browser/metrics/metrics_service_observer.h', On 2014/05/08 05:40:19, Lei Zhang wrote: > nit: alphabetical order. _ob should follow _an Done.
On 2014/05/08 06:38:24, bolian wrote: > Thanks for the review! > > - Will there be another class that implements MetricsLogObserver in the near > future? > > NO. So what if we just got rid of MetricsServiceObserver and ChromeBrowserMetricsServiceObserver? Instead, just call et::NetworkChangeNotifier::LogOperatorCodeHistogram() in metrics_service.cc in NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you haven't lost too much because there's no need for more MetricsServiceObserver? Later on, when metrics componentization occurs, if this doesn't work, the person doing the refactoring will understand the future situation better than we do now and rearrange the code appropriately. That person will have just a function to deal with, rather than 2 classes.
On 2014/05/08 07:22:46, Lei Zhang wrote: > On 2014/05/08 06:38:24, bolian wrote: > > Thanks for the review! > > > > - Will there be another class that implements MetricsLogObserver in the near > > future? > > > > NO. > > So what if we just got rid of MetricsServiceObserver and > ChromeBrowserMetricsServiceObserver? Instead, just call > et::NetworkChangeNotifier::LogOperatorCodeHistogram() in metrics_service.cc in > NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you haven't > lost too much because there's no need for more MetricsServiceObserver? > > Later on, when metrics componentization occurs, if this doesn't work, the person > doing the refactoring will understand the future situation better than we do now > and rearrange the code appropriately. That person will have just a function to > deal with, rather than 2 classes. We're planning to do the componentization this quarter. We have a week-long hackathon scheduled for this later this Month and I'm starting the detailed design doc for it this week. So it's not a hypothetical thing happening some number of years down the road. So I'd prefer the observer interface and doing things outside of MetricsService, otherwise this stuff will need to be refactored again very soon.
On 2014/05/08 15:23:49, Alexei Svitkine wrote: > On 2014/05/08 07:22:46, Lei Zhang wrote: > > On 2014/05/08 06:38:24, bolian wrote: > > > Thanks for the review! > > > > > > - Will there be another class that implements MetricsLogObserver in the near > > > future? > > > > > > NO. > > > > So what if we just got rid of MetricsServiceObserver and > > ChromeBrowserMetricsServiceObserver? Instead, just call > > et::NetworkChangeNotifier::LogOperatorCodeHistogram() in metrics_service.cc in > > NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you haven't > > lost too much because there's no need for more MetricsServiceObserver? > > > > Later on, when metrics componentization occurs, if this doesn't work, the > person > > doing the refactoring will understand the future situation better than we do > now > > and rearrange the code appropriately. That person will have just a function to > > deal with, rather than 2 classes. > > We're planning to do the componentization this quarter. We have a week-long > hackathon scheduled for this later this Month and I'm starting the detailed > design doc for it this week. So it's not a hypothetical thing happening some > number of years down the road. > > So I'd prefer the observer interface and doing things outside of MetricsService, > otherwise this stuff will need to be refactored again very soon. I am neutral on either way. I just hope we can settle this down quickly.
On 2014/05/08 15:23:49, Alexei Svitkine wrote: > On 2014/05/08 07:22:46, Lei Zhang wrote: > > On 2014/05/08 06:38:24, bolian wrote: > > > Thanks for the review! > > > > > > - Will there be another class that implements MetricsLogObserver in the near > > > future? > > > > > > NO. > > > > So what if we just got rid of MetricsServiceObserver and > > ChromeBrowserMetricsServiceObserver? Instead, just call > > et::NetworkChangeNotifier::LogOperatorCodeHistogram() in metrics_service.cc in > > NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you haven't > > lost too much because there's no need for more MetricsServiceObserver? > > > > Later on, when metrics componentization occurs, if this doesn't work, the > person > > doing the refactoring will understand the future situation better than we do > now > > and rearrange the code appropriately. That person will have just a function to > > deal with, rather than 2 classes. > > We're planning to do the componentization this quarter. We have a week-long > hackathon scheduled for this later this Month and I'm starting the detailed > design doc for it this week. So it's not a hypothetical thing happening some > number of years down the road. > > So I'd prefer the observer interface and doing things outside of MetricsService, > otherwise this stuff will need to be refactored again very soon. Do you know for sure an observer interface is what you want when the refactoring happens? Since there's only a single observer, have you considered adding a base::Callback instead, with set/reset callback methods instead of add/remove observer methods? If you really want this to happen outside of MetricsService, consider adding it to ChromeBrowserMainExtraPartsMetrics::PreCreateThread() instead of putting it in ChromeBrowserMain. ChromeBrowserMainExtraPartsMetrics::PreCreateThread() happens very soon after MetricsService creation.
On 2014/05/08 18:32:30, Lei Zhang wrote: > On 2014/05/08 15:23:49, Alexei Svitkine wrote: > > On 2014/05/08 07:22:46, Lei Zhang wrote: > > > On 2014/05/08 06:38:24, bolian wrote: > > > > Thanks for the review! > > > > > > > > - Will there be another class that implements MetricsLogObserver in the > near > > > > future? > > > > > > > > NO. > > > > > > So what if we just got rid of MetricsServiceObserver and > > > ChromeBrowserMetricsServiceObserver? Instead, just call > > > et::NetworkChangeNotifier::LogOperatorCodeHistogram() in metrics_service.cc > in > > > NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you > haven't > > > lost too much because there's no need for more MetricsServiceObserver? > > > > > > Later on, when metrics componentization occurs, if this doesn't work, the > > person > > > doing the refactoring will understand the future situation better than we do > > now > > > and rearrange the code appropriately. That person will have just a function > to > > > deal with, rather than 2 classes. > > > > We're planning to do the componentization this quarter. We have a week-long > > hackathon scheduled for this later this Month and I'm starting the detailed > > design doc for it this week. So it's not a hypothetical thing happening some > > number of years down the road. > > > > So I'd prefer the observer interface and doing things outside of > MetricsService, > > otherwise this stuff will need to be refactored again very soon. > > Do you know for sure an observer interface is what you want when the refactoring > happens? Since there's only a single observer, have you considered adding a > base::Callback instead, with set/reset callback methods instead of add/remove > observer methods? That's a good point: A CallbackList is probably better than an ObserverList.
I think we'll want more events to be observed in the future. For example, check the code at the end of MetricsService::OnURLFetchComplete() that calls chrome_browser_net::CollectNetworkStats() and friends. This code should be moved out of MetricsService, so we'll want to have a OnMetricsLogUploaded() observer method. If we go with the observer class, we can then expand it to add that method. Or we could make a separate callback list for the above, but it seems messier. On Thu, May 8, 2014 at 2:36 PM, <isherman@chromium.org> wrote: > On 2014/05/08 18:32:30, Lei Zhang wrote: > >> On 2014/05/08 15:23:49, Alexei Svitkine wrote: >> > On 2014/05/08 07:22:46, Lei Zhang wrote: >> > > On 2014/05/08 06:38:24, bolian wrote: >> > > > Thanks for the review! >> > > > >> > > > - Will there be another class that implements MetricsLogObserver in >> the >> near >> > > > future? >> > > > >> > > > NO. >> > > >> > > So what if we just got rid of MetricsServiceObserver and >> > > ChromeBrowserMetricsServiceObserver? Instead, just call >> > > et::NetworkChangeNotifier::LogOperatorCodeHistogram() in >> > metrics_service.cc > >> in >> > > NotifyOnDidCreateMetricsLog(). Wouldn't that be a lot simpler and you >> haven't >> > > lost too much because there's no need for more MetricsServiceObserver? >> > > >> > > Later on, when metrics componentization occurs, if this doesn't work, >> the >> > person >> > > doing the refactoring will understand the future situation better >> than we >> > do > >> > now >> > > and rearrange the code appropriately. That person will have just a >> > function > >> to >> > > deal with, rather than 2 classes. >> > >> > We're planning to do the componentization this quarter. We have a >> week-long >> > hackathon scheduled for this later this Month and I'm starting the >> detailed >> > design doc for it this week. So it's not a hypothetical thing happening >> some >> > number of years down the road. >> > >> > So I'd prefer the observer interface and doing things outside of >> MetricsService, >> > otherwise this stuff will need to be refactored again very soon. >> > > Do you know for sure an observer interface is what you want when the >> > refactoring > >> happens? Since there's only a single observer, have you considered adding >> a >> base::Callback instead, with set/reset callback methods instead of >> add/remove >> observer methods? >> > > That's a good point: A CallbackList is probably better than an > ObserverList. > > https://codereview.chromium.org/253203002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/05/08 18:42:29, Alexei Svitkine wrote: > I think we'll want more events to be observed in the future. > > For example, check the code at the end of > MetricsService::OnURLFetchComplete() that calls > chrome_browser_net::CollectNetworkStats() and friends. This code should be > moved out of MetricsService, so we'll want to have a OnMetricsLogUploaded() > observer method. If we go with the observer class, we can then expand it to > add that method. > > Or we could make a separate callback list for the above, but it seems > messier. Ok, then keep the Observer. I asked Bolian and he said no... Just move the ChromeBrowserMain bits to ChromeBrowserMainExtraPartsMetrics then?
Although now that I'm looking at MetricsService more closely for the component refactoring, it occurs to me that this histogram is not very much unlike some other histograms that we record on every UMA log that are done in a different spot. For example, the MetricsService::StartFinalLogInfoCollection() code uses a helper MemoryDetails object to gather memory details (which that helper class logs as histograms). So it sounds like it's a very similar use case and perhaps the Observer interface as implemented in this CL isn't the best fit. Instead, we should probably have a generic mechanism to register extra histogram collection tasks that are asynchronous to record extra histogram data. And both this and the MemoryDetails code should use the same interface. Now, I realise that this is likely quite a bit of work to do at this point if you're trying to land before the branch cut. So maybe for the sake of making it into the branch, the right thing to do for this CL is similar to what thestig@ suggested - to have MetricsService call the specific chrome net function directly - from StartFinalLogInfoCollection(). Then, when we build the generic mechanism, we can switch the code to it (in M37). Ilya, WDYT?
On 2014/05/08 19:55:18, Alexei Svitkine wrote: > Although now that I'm looking at MetricsService more closely for the component > refactoring, it occurs to me that this histogram is not very much unlike some > other histograms that we record on every UMA log that are done in a different > spot. > > For example, the MetricsService::StartFinalLogInfoCollection() code uses a > helper MemoryDetails object to gather memory details (which that helper class > logs as histograms). > > So it sounds like it's a very similar use case and perhaps the Observer > interface as implemented in this CL isn't the best fit. > > Instead, we should probably have a generic mechanism to register extra histogram > collection tasks that are asynchronous to record extra histogram data. And both > this and the MemoryDetails code should use the same interface. > > Now, I realise that this is likely quite a bit of work to do at this point if > you're trying to land before the branch cut. So maybe for the sake of making it > into the branch, the right thing to do for this CL is similar to what thestig@ > suggested - to have MetricsService call the specific chrome net function > directly - from StartFinalLogInfoCollection(). > > Then, when we build the generic mechanism, we can switch the code to it (in > M37). > > Ilya, WDYT? I have updated the CL to create the observer in ChromeBrowserMainExtraPartsMetrics::PostBrowserStart(). But if we have an agreement, I don't mind removing that and simply call LogOperatorCodeHistogram directly in a proper place in MetricsService.
Generally lgtm. https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:162: metrics_service_observer_.reset(new ChromeBrowserMetricsServiceObserver()); If you want this to happen earlier, so it is closer in behavior to the previous patch set, feel free to add ChromeBrowserMainExtraPartsMetrics::PreCreateThread(). https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:32: scoped_ptr<ChromeBrowserMetricsServiceObserver> metrics_service_observer_; Android-only?
On 2014/05/08 20:22:16, bolian wrote: > On 2014/05/08 19:55:18, Alexei Svitkine wrote: > > Although now that I'm looking at MetricsService more closely for the component > > refactoring, it occurs to me that this histogram is not very much unlike some > > other histograms that we record on every UMA log that are done in a different > > spot. > > > > For example, the MetricsService::StartFinalLogInfoCollection() code uses a > > helper MemoryDetails object to gather memory details (which that helper class > > logs as histograms). > > > > So it sounds like it's a very similar use case and perhaps the Observer > > interface as implemented in this CL isn't the best fit. > > > > Instead, we should probably have a generic mechanism to register extra > histogram > > collection tasks that are asynchronous to record extra histogram data. And > both > > this and the MemoryDetails code should use the same interface. > > > > Now, I realise that this is likely quite a bit of work to do at this point if > > you're trying to land before the branch cut. So maybe for the sake of making > it > > into the branch, the right thing to do for this CL is similar to what thestig@ > > suggested - to have MetricsService call the specific chrome net function > > directly - from StartFinalLogInfoCollection(). > > > > Then, when we build the generic mechanism, we can switch the code to it (in > > M37). > > > > Ilya, WDYT? > > I have updated the CL to create the observer in > ChromeBrowserMainExtraPartsMetrics::PostBrowserStart(). > But if we have an agreement, I don't mind removing that and simply call > LogOperatorCodeHistogram directly in a proper place in MetricsService. I'm fine with punting the observer interface to a separate CL. As long as the business logic of your specific metric does not leak into the MetricsService, I'm fine with calling LogOperatorCodeHistogram() from the appropriate places in MetricsService. I'd recommend routing through a simple wrapper function with a name like NotifyOnDidStartNewMetricsSession (or like the existing Notify...() function name you already have), so that the semantics are more immediately clear to anyone who comes along to refactor the code.
Okay, chatted with Ilya offline re: my previous comment. Ilya convinced me that it makes sense to have both: histograms that are recorded at the start of the log and those that are recorded at the end. So it makes sense to add the new interface for the "start" case. Have one comment on the CL, otherwise looks good. Thanks! https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1430: NotifyOnDidCreateMetricsLog(); Remove this line. Possibly replace with a comment. It's not correct to do this here, since the stability log describes stats from the _previous_ session (and generally has almost no histograms except for very specific ones that also describe stats about the previous session). The next line loads the previously saved system profile from then. So it's not correct to record these stats there.
Thanks! I am keep the current observer. https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:162: metrics_service_observer_.reset(new ChromeBrowserMetricsServiceObserver()); From what I tested this seems fine and it happened before the first metrics log. https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.h:32: scoped_ptr<ChromeBrowserMetricsServiceObserver> metrics_service_observer_; I am trying to reduce the #if artifacts, so that I only have it once in the .cc file with the minor penalty of the additional scoped_ptr for other platforms. Otherwise, I have two #if define's here (header and the var) and two others in the .cc file. Readability wise, this seems better. https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/253203002/diff/300001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service.cc:1430: NotifyOnDidCreateMetricsLog(); On 2014/05/08 21:00:56, Alexei Svitkine wrote: > Remove this line. Possibly replace with a comment. > > It's not correct to do this here, since the stability log describes stats from > the _previous_ session (and generally has almost no histograms except for very > specific ones that also describe stats about the previous session). The next > line loads the previously saved system profile from then. > > So it's not correct to record these stats there. Done.
lgtm % comment Please wait for Ilya's review as well. https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:18: MetricsServiceObserver() {} Don't inline implementations in header files for ctors and dtors. Instead, make a .cc file and put them there.
https://chromiumcodereview.appspot.com/253203002/diff/320001/chrome/browser/m... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://chromiumcodereview.appspot.com/253203002/diff/320001/chrome/browser/m... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:160: // We only need this histogram for Android for now. nit: s/histogram/ https://chromiumcodereview.appspot.com/253203002/diff/320001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/253203002/diff/320001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:10696: + changed to a mobile network. A value of zero means a non-mobile network or nit: Is the metric also logged when the network connection is changed *from* a mobile network?
Sorry, meant to say: LGTM with the remaining nits addressed. Thanks for bearing with us figuring out what design to go with.
Thank you very much for the review! https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc (right): https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... chrome/browser/metrics/chrome_browser_main_extra_parts_metrics.cc:160: // We only need this histogram for Android for now. On 2014/05/08 22:43:22, Ilya Sherman wrote: > nit: s/histogram/ Done. https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... File chrome/browser/metrics/metrics_service_observer.h (right): https://codereview.chromium.org/253203002/diff/320001/chrome/browser/metrics/... chrome/browser/metrics/metrics_service_observer.h:18: MetricsServiceObserver() {} On 2014/05/08 22:26:54, Alexei Svitkine wrote: > Don't inline implementations in header files for ctors and dtors. > > Instead, make a .cc file and put them there. Done. https://codereview.chromium.org/253203002/diff/320001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/253203002/diff/320001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:10696: + changed to a mobile network. A value of zero means a non-mobile network or Done. Removed "to a mobile network".
Np at all. Thanks for the comments.
Hello Misha, Could you take a look at the //net/base part of the change? Abstracted a new func LogOperatorCodeHistogram and used also in chrome/browser/chrome_browser_metrics_service_observer.cc Thanks, Bolian
net/base/ lgtm
The CQ bit was checked by bolian@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/253203002/340001
The CQ bit was checked by bolian@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/253203002/360001
The CQ bit was unchecked by bolian@chromium.org
The CQ bit was checked by bolian@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bolian@chromium.org/253203002/380001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
Message was sent while issue was closed.
Change committed as 269627 |
