Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1033493002:
Histogram for UMA log upload connection type. (Closed)
|
Created:
5 years, 9 months ago by gayane -on leave until 09-2017 Modified:
5 years, 9 months ago CC:
chromium-reviews, gunsch+watch_chromium.org, lcwu+watch_chromium.org, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org, Randy Smith (Not in Mondays) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHistogram for UMA log upload connection type.
Added a histogram to log the network connection type for each successful metrics log upload. Will be used to monitor cellular uploads on Chrome on Android.
BUG=455847
Committed: https://crrev.com/4849a80712b9d870522c50968a30ca7f370894c5
Cr-Commit-Position: refs/heads/master@{#322593}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 12
Patch Set 3 : Histogram type changed, refactoring network_metrics_provider_ + nits #
Total comments: 10
Patch Set 4 : Recording ConnectionType histogram in NetMetricsLogUploader #
Total comments: 6
Patch Set 5 : Comment for enum added + nits #
Total comments: 2
Patch Set 6 : Comment fix #
Messages
Total messages: 25 (8 generated)
gayane@chromium.org changed reviewers: + isherman@chromium.org
Please have a look. I have added a histogram for recording a connection type for each successful UMA log upload. For that I have refactored ChromeMetricsServiceClient to store NetworkMetricsProvider instead of previously only the callback on isCellularConnection() function.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:129: weak_ptr_factory_(this) { Please initialize the metrics provider here. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:213: network_metrics_provider_->GetConnectionType()); FWIW, this histogram doesn't really need to be sparse. UMA_HISTOGRAM_ENUMERATION might be more appropriate here. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:213: network_metrics_provider_->GetConnectionType()); The connection type is already recorded in the system profile. Why do we need to record it as a histogram as well? https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:315: // GetConnectionCallback should be called from non const pointer. I don't understand this comment. Why does this need to be documented? https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.h:177: const metrics::NetworkMetricsProvider* network_metrics_provider_; Please move this to be defined near where the other metrics providers are, and document the lifetime expectations. https://codereview.chromium.org/1033493002/diff/40001/components/metrics/metr... File components/metrics/metrics_service_client.h (right): https://codereview.chromium.org/1033493002/diff/40001/components/metrics/metr... components/metrics/metrics_service_client.h:53: // Called by the metrics service when a log has been uploaded. Please update the documentation for this method.
Changed histogram type to enumeration and refactored network_metrics_provider_ in ChromeMetricsServiceClient to be similar to other metrics providers https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:129: weak_ptr_factory_(this) { On 2015/03/24 00:14:23, Ilya Sherman wrote: > Please initialize the metrics provider here. Done. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:213: network_metrics_provider_->GetConnectionType()); On 2015/03/24 00:14:23, Ilya Sherman wrote: > The connection type is already recorded in the system profile. Why do we need > to record it as a histogram as well? As discussed, system profile records the connection type during the collection of the records while we are interested to know the connection type used for the log upload. Note: as histogram is recorded after the upload it might not be exact but should be pretty close. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:213: network_metrics_provider_->GetConnectionType()); On 2015/03/24 00:14:23, Ilya Sherman wrote: > FWIW, this histogram doesn't really need to be sparse. > UMA_HISTOGRAM_ENUMERATION might be more appropriate here. Done. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:315: // GetConnectionCallback should be called from non const pointer. On 2015/03/24 00:14:23, Ilya Sherman wrote: > I don't understand this comment. Why does this need to be documented? As network_metrics_provider_ as a const pointer I couldn't use it to call GetConnectionCallback as its not const. However, I have removed const from network_metrics_provider_ to be similar to other metrics providers. As a result moved profiler_metrics_provider_ back where it was and used network_metrics_provider_. https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.h (right): https://codereview.chromium.org/1033493002/diff/40001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.h:177: const metrics::NetworkMetricsProvider* network_metrics_provider_; On 2015/03/24 00:14:23, Ilya Sherman wrote: > Please move this to be defined near where the other metrics providers are, and > document the lifetime expectations. Done. https://codereview.chromium.org/1033493002/diff/40001/components/metrics/metr... File components/metrics/metrics_service_client.h (right): https://codereview.chromium.org/1033493002/diff/40001/components/metrics/metr... components/metrics/metrics_service_client.h:53: // Called by the metrics service when a log has been uploaded. On 2015/03/24 00:14:23, Ilya Sherman wrote: > Please update the documentation for this method. Done.
Thanks, Gayane. https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:16: #include "base/metrics/sparse_histogram.h" nit: No longer needed? https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:216: metrics::SystemProfileProto::Network::CONNECTION_BLUETOOTH + 1); Rather than using the enum type from the protocol buffer, which is really not designed to be used in this way, how about skipping the network_metrics_provider_ and just calling net::NetworkChangeNotifier::GetConnectionType() directly? In fact, you could probably do this from the metrics component's code, rather than needing to change the clients. Actually, it might also be worth defining a separate enum specifically for backing this histogram, since (a) the protobuf isn't guaranteed to preserve CONNECTION_BLUETOOTH as the final element, and (b) the net layer code isn't guaranteed to treat its enum as append-only. https://codereview.chromium.org/1033493002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1033493002/diff/60001/components/metrics/metr... components/metrics/metrics_service.cc:1000: client_->OnLogUploadComplete(upload_succeeded); I think we might be able to significantly reduce the size of this CL by asking the log_uploader_ to log the network metrics, rather than asking the client to do so. If we don't care about the Error 400 check above, we could even just directly log the metric in NetMetricsLogUploader::OnURLFetchComplete(). WDYT? https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40480: + <owner>asvitkine@chromium.org</owner> nit: Please list yourself as an owner as well, unless you have a reason not to? https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40482: + Log the network connection type for each successful metrics log upload. nit: "Log the" -> "The"
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
Did as suggested: moved the histogram to NetMetricsLogUploader, but didn't create new enum. Used net::NetworkChangeNotifier::ConnectionType https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... File chrome/browser/metrics/chrome_metrics_service_client.cc (right): https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:16: #include "base/metrics/sparse_histogram.h" On 2015/03/24 21:55:44, Ilya Sherman wrote: > nit: No longer needed? Done. https://codereview.chromium.org/1033493002/diff/60001/chrome/browser/metrics/... chrome/browser/metrics/chrome_metrics_service_client.cc:216: metrics::SystemProfileProto::Network::CONNECTION_BLUETOOTH + 1); On 2015/03/24 21:55:44, Ilya Sherman wrote: > Rather than using the enum type from the protocol buffer, which is really not > designed to be used in this way, how about skipping the > network_metrics_provider_ and just calling > net::NetworkChangeNotifier::GetConnectionType() directly? In fact, you could > probably do this from the metrics component's code, rather than needing to > change the clients. > > Actually, it might also be worth defining a separate enum specifically for > backing this histogram, since (a) the protobuf isn't guaranteed to preserve > CONNECTION_BLUETOOTH as the final element, and (b) the net layer code isn't > guaranteed to treat its enum as append-only. For the enum eventually I have used net::NetworkChangeNotifier::ConnectionType. https://codereview.chromium.org/1033493002/diff/60001/components/metrics/metr... File components/metrics/metrics_service.cc (right): https://codereview.chromium.org/1033493002/diff/60001/components/metrics/metr... components/metrics/metrics_service.cc:1000: client_->OnLogUploadComplete(upload_succeeded); On 2015/03/24 21:55:44, Ilya Sherman wrote: > I think we might be able to significantly reduce the size of this CL by asking > the log_uploader_ to log the network metrics, rather than asking the client to > do so. If we don't care about the Error 400 check above, we could even just > directly log the metric in NetMetricsLogUploader::OnURLFetchComplete(). WDYT? I like this. Done. https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40480: + <owner>asvitkine@chromium.org</owner> On 2015/03/24 21:55:45, Ilya Sherman wrote: > nit: Please list yourself as an owner as well, unless you have a reason not to? Done. https://codereview.chromium.org/1033493002/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40482: + Log the network connection type for each successful metrics log upload. On 2015/03/24 21:55:44, Ilya Sherman wrote: > nit: "Log the" -> "The" Done.
Thanks! https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:20: net::NetworkChangeNotifier::CONNECTION_LAST); If you reuse this enum, please add a comment above the enum definition warning readers that the enum is used to back a histogram, and hence should be treated as append-only. https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:22: } nit: Please leave a blank line after this one. https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:23: } nit: Please add a comment " // namespace"
Comment for enum added + nits https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... File components/metrics/net/net_metrics_log_uploader.cc (right): https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:20: net::NetworkChangeNotifier::CONNECTION_LAST); On 2015/03/25 23:10:00, Ilya Sherman wrote: > If you reuse this enum, please add a comment above the enum definition warning > readers that the enum is used to back a histogram, and hence should be treated > as append-only. Done. https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:22: } On 2015/03/25 23:10:00, Ilya Sherman wrote: > nit: Please leave a blank line after this one. Done. https://codereview.chromium.org/1033493002/diff/120001/components/metrics/net... components/metrics/net/net_metrics_log_uploader.cc:23: } On 2015/03/25 23:10:00, Ilya Sherman wrote: > nit: Please add a comment " // namespace" Done.
Patchset #5 (id:140001) has been deleted
gayane@chromium.org changed reviewers: + rdsmith@chromium.org
rdsmith@chromium.org: Please review changes in net/base/network_change_notifier.h
rdsmith@chromium.org changed reviewers: + pauljensen@chromium.org - rdsmith@chromium.org
Lateralled to Paul; it looks innocuous, but I care about getting documentation right and it's his code.
https://codereview.chromium.org/1033493002/diff/160001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/1033493002/diff/160001/net/base/network_chang... net/base/network_change_notifier.h:45: // Used to record the connection type used for uploading UMA logs in UMA This is used for lots and lots of things, including surfacing to web pages via netinfo. This sentence makes it sounds like it's used just for UMA typing. Can we adjust this to be clearer, perhaps something like "New enum values should only be added to the end of the enum and no values should be modified or reused, as this is reported via UMA."
Comment fixed. https://codereview.chromium.org/1033493002/diff/160001/net/base/network_chang... File net/base/network_change_notifier.h (right): https://codereview.chromium.org/1033493002/diff/160001/net/base/network_chang... net/base/network_change_notifier.h:45: // Used to record the connection type used for uploading UMA logs in UMA On 2015/03/26 19:03:35, pauljensen wrote: > This is used for lots and lots of things, including surfacing to web pages via > netinfo. This sentence makes it sounds like it's used just for UMA typing. Can > we adjust this to be clearer, perhaps something like "New enum values should > only be added to the end of the enum and no values should be modified or reused, > as this is reported via UMA." Done.
LGTM. Thanks, Gayane.
net/ lgtm. Are you aware we already have lots of NetworkChangeNotifier histograms describing all kinds of things, including connection type?
On 2015/03/27 11:25:16, pauljensen wrote: > net/ lgtm. Are you aware we already have lots of NetworkChangeNotifier > histograms describing all kinds of things, including connection type? In this case, I want to record what connection is used for uploading UMA logs, because I am changing the behavior of UMA log uploads on cellular networks and I want to monitor it.
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1033493002/180001
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4849a80712b9d870522c50968a30ca7f370894c5 Cr-Commit-Position: refs/heads/master@{#322593} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
