Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2778203003:
WebView: Create UMA uploader (Closed)
|
Created:
3 years, 8 months ago by paulmiller Modified:
3 years, 8 months ago CC:
chromium-reviews, android-webview-reviews_chromium.org, asvitkine+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebView: Create UMA uploader
WebView uses an unspecified platform mechanism to upload UMA logs,
rather than the typical cronet path. This creates a WebView-specific
MetricsLogUploader, which sends logs to PlatformServiceBridge, which
connects to the platform mechanism.
BUG=698047
Review-Url: https://codereview.chromium.org/2778203003
Cr-Commit-Position: refs/heads/master@{#460649}
Committed: https://chromium.googlesource.com/chromium/src/+/a5febff94dd70a488bd13ace5a182a3b3d11156b
Patch Set 1 #
Total comments: 12
Patch Set 2 : WebView: Create UMA uploader #
Total comments: 7
Patch Set 3 : nits #Patch Set 4 : implement RegisterAwMetricsLogUploader #Patch Set 5 : unimplemnet RegisterAwMetricsLogUploader #Messages
Total messages: 30 (12 generated)
Description was changed from ========== WebView: Create UMA uploader WebView uses an unspecified platform mechanism to upload UMA logs, rather than the typical cronet path. This creates a WebView-specific MetricsLogUploader, which sends logs to PlatformServiceBridge, which connects to the platform mechanism. BUG=698047 ========== to ========== WebView: Create UMA uploader WebView uses an unspecified platform mechanism to upload UMA logs, rather than the typical cronet path. This creates a WebView-specific MetricsLogUploader, which sends logs to PlatformServiceBridge, which connects to the platform mechanism. BUG=698047 ==========
paulmiller@chromium.org changed reviewers: + asvitkine@chromium.org, michaelbai@chromium.org
On 2017/03/28 23:36:11, paulmiller wrote: > mailto:paulmiller@chromium.org changed reviewers: > + mailto:asvitkine@chromium.org, mailto:michaelbai@chromium.org PTAL, Alexei for metrics/ and Michael for android_webview/. The basic idea is to take data from the native/metrics bit and send it to the Java/WebView PlatformServiceBridge, which is overridden downstream. I added SimpleMetricsLogUploader to avoid adding a dependency from WebView onto third_party/zlib.
https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance(null).logMetrics(data); If you ever want to write a test for this, you will get problem. https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... File android_webview/native/aw_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... android_webview/native/aw_metrics_log_uploader.h:25: const base::Callback<void(int)> on_upload_complete_; add DISALLOW_COPY_AND_ASSIGN()?
https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance(null).logMetrics(data); On 2017/03/29 01:29:55, michaelbai wrote: > If you ever want to write a test for this, you will get problem. What do you mean, test uploadLog independently from WebViewChromiumFactoryProvider? The tester could always call getInstance itself before calling uploadLog. The alternative would be to plumb the Context into this class, which would be ugly, especially since it wouldn't ever be needed in a non-test scenario.
https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... File android_webview/native/aw_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... android_webview/native/aw_metrics_log_uploader.h:21: const base::Callback<void(int)>& on_upload_complete); Nit: Add an explicit dtor and leave an empty line between ctor/dtor and the method. Above the method, per convention put // ::metrics::SimpleMetricsLogUploader: https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... File components/metrics/simple_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... components/metrics/simple_metrics_log_uploader.h:17: // compression or hashes. Do we really need this? Seems all this is doing is adding a convenience GzipUncompress() call - why not just do that in AwMetricsLogUploader directly? You can still have the separate method split.
https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... File components/metrics/simple_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... components/metrics/simple_metrics_log_uploader.h:17: // compression or hashes. On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > Do we really need this? Seems all this is doing is adding a convenience > GzipUncompress() call - why not just do that in AwMetricsLogUploader directly? > You can still have the separate method split. I did that initially, but checkdeps complained. Rather than add a dependency from WebView onto third_party/zlib, I added SimpleMetricsLogUploader. Loose coupling, and all that.
https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... File components/metrics/simple_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... components/metrics/simple_metrics_log_uploader.h:17: // compression or hashes. On 2017/03/29 17:39:32, paulmiller wrote: > On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > > Do we really need this? Seems all this is doing is adding a convenience > > GzipUncompress() call - why not just do that in AwMetricsLogUploader directly? > > You can still have the separate method split. > > I did that initially, but checkdeps complained. Rather than add a dependency > from WebView onto third_party/zlib, I added SimpleMetricsLogUploader. Loose > coupling, and all that. Given this code is not being used by anything else, I think it should still live in webview. You can just add zlib to the DEPS whitelist for webview.
https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance(null).logMetrics(data); On 2017/03/29 01:59:41, paulmiller wrote: > On 2017/03/29 01:29:55, michaelbai wrote: > > If you ever want to write a test for this, you will get problem. > > What do you mean, test uploadLog independently from > WebViewChromiumFactoryProvider? The tester could always call getInstance itself > before calling uploadLog. The alternative would be to plumb the Context into > this class, which would be ugly, especially since it wouldn't ever be needed in > a non-test scenario. Well, you can wrote test like what you suggested, here are some suggestions - Looking how this methods used by others, AwMinidumpUploaderDelegate.java does pass the context into PlatformServiceBridge.getInstance(). - If you don't want to pass context,you may add a new method getInstance() which has no parameter. I think the current getInstance() should be renamed to createInstance(), but we can rename it in separated patch.
https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... File android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: PlatformServiceBridge.getInstance(null).logMetrics(data); On 2017/03/29 18:04:48, michaelbai wrote: > On 2017/03/29 01:59:41, paulmiller wrote: > > On 2017/03/29 01:29:55, michaelbai wrote: > > > If you ever want to write a test for this, you will get problem. > > > > What do you mean, test uploadLog independently from > > WebViewChromiumFactoryProvider? The tester could always call getInstance > itself > > before calling uploadLog. The alternative would be to plumb the Context into > > this class, which would be ugly, especially since it wouldn't ever be needed > in > > a non-test scenario. > > Well, you can wrote test like what you suggested, here are some suggestions > - Looking how this methods used by others, AwMinidumpUploaderDelegate.java does > pass the context into PlatformServiceBridge.getInstance(). > > - If you don't want to pass context,you may add a new method getInstance() which > has no parameter. I think the current getInstance() should be renamed to > createInstance(), but we can rename it in separated patch. Okay, I'll do a separate patch. https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... File android_webview/native/aw_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... android_webview/native/aw_metrics_log_uploader.h:21: const base::Callback<void(int)>& on_upload_complete); On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > Nit: Add an explicit dtor and leave an empty line between ctor/dtor and the > method. Above the method, per convention put // > ::metrics::SimpleMetricsLogUploader: Will do. What's the reason for the explicit destructor? https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... android_webview/native/aw_metrics_log_uploader.h:25: const base::Callback<void(int)> on_upload_complete_; On 2017/03/29 01:29:55, michaelbai wrote: > add DISALLOW_COPY_AND_ASSIGN()? Will do. https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... File components/metrics/simple_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... components/metrics/simple_metrics_log_uploader.h:17: // compression or hashes. On 2017/03/29 17:41:57, Alexei Svitkine (slow) wrote: > On 2017/03/29 17:39:32, paulmiller wrote: > > On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > > > Do we really need this? Seems all this is doing is adding a convenience > > > GzipUncompress() call - why not just do that in AwMetricsLogUploader > directly? > > > You can still have the separate method split. > > > > I did that initially, but checkdeps complained. Rather than add a dependency > > from WebView onto third_party/zlib, I added SimpleMetricsLogUploader. Loose > > coupling, and all that. > > Given this code is not being used by anything else, I think it should still live > in webview. You can just add zlib to the DEPS whitelist for webview. Will do. I could go either way. On one hand, there's the fluffy extra abstraction of SimpleMetricsLogUploader; on the other is making WebView aware of gzip, which is arguably an implementation detail of the metrics code.
On 2017/03/29 18:33:28, paulmiller wrote: > https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... > File > android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java > (right): > > https://codereview.chromium.org/2778203003/diff/1/android_webview/java/src/or... > android_webview/java/src/org/chromium/android_webview/AwMetricsLogUploader.java:17: > PlatformServiceBridge.getInstance(null).logMetrics(data); > On 2017/03/29 18:04:48, michaelbai wrote: > > On 2017/03/29 01:59:41, paulmiller wrote: > > > On 2017/03/29 01:29:55, michaelbai wrote: > > > > If you ever want to write a test for this, you will get problem. > > > > > > What do you mean, test uploadLog independently from > > > WebViewChromiumFactoryProvider? The tester could always call getInstance > > itself > > > before calling uploadLog. The alternative would be to plumb the Context into > > > this class, which would be ugly, especially since it wouldn't ever be needed > > in > > > a non-test scenario. > > > > Well, you can wrote test like what you suggested, here are some suggestions > > - Looking how this methods used by others, AwMinidumpUploaderDelegate.java > does > > pass the context into PlatformServiceBridge.getInstance(). > > > > - If you don't want to pass context,you may add a new method getInstance() > which > > has no parameter. I think the current getInstance() should be renamed to > > createInstance(), but we can rename it in separated patch. > > Okay, I'll do a separate patch. > > https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... > File android_webview/native/aw_metrics_log_uploader.h (right): > > https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... > android_webview/native/aw_metrics_log_uploader.h:21: const > base::Callback<void(int)>& on_upload_complete); > On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > > Nit: Add an explicit dtor and leave an empty line between ctor/dtor and the > > method. Above the method, per convention put // > > ::metrics::SimpleMetricsLogUploader: > > Will do. What's the reason for the explicit destructor? > > https://codereview.chromium.org/2778203003/diff/1/android_webview/native/aw_m... > android_webview/native/aw_metrics_log_uploader.h:25: const > base::Callback<void(int)> on_upload_complete_; > On 2017/03/29 01:29:55, michaelbai wrote: > > add DISALLOW_COPY_AND_ASSIGN()? > > Will do. > > https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... > File components/metrics/simple_metrics_log_uploader.h (right): > > https://codereview.chromium.org/2778203003/diff/1/components/metrics/simple_m... > components/metrics/simple_metrics_log_uploader.h:17: // compression or hashes. > On 2017/03/29 17:41:57, Alexei Svitkine (slow) wrote: > > On 2017/03/29 17:39:32, paulmiller wrote: > > > On 2017/03/29 17:32:23, Alexei Svitkine (slow) wrote: > > > > Do we really need this? Seems all this is doing is adding a convenience > > > > GzipUncompress() call - why not just do that in AwMetricsLogUploader > > directly? > > > > You can still have the separate method split. > > > > > > I did that initially, but checkdeps complained. Rather than add a dependency > > > from WebView onto third_party/zlib, I added SimpleMetricsLogUploader. Loose > > > coupling, and all that. > > > > Given this code is not being used by anything else, I think it should still > live > > in webview. You can just add zlib to the DEPS whitelist for webview. > > Will do. I could go either way. On one hand, there's the fluffy extra > abstraction of SimpleMetricsLogUploader; on the other is making WebView aware of > gzip, which is arguably an implementation detail of the metrics code. PTAL at #2
android_webview LGTM
lgtm https://codereview.chromium.org/2778203003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:72: // Takes an uncompressed, serialized UMA proto. Nit: Add a sentence at the beginning to explain what this does - beyond just documenting the param. https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... File android_webview/native/aw_metrics_log_uploader.cc (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... android_webview/native/aw_metrics_log_uploader.cc:28: compression::GzipUncompress(compressed_log_data, &log_data); I agree about your point about this leaking implementation details from components/metrics. If you prefer, an option would be to make a wrapper for this in components/metrics that's called "DecodeLogData" or something like that - and then this can call it instead of gzip directly. (Up to you - can also done in a separate CL). https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... File android_webview/native/aw_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... android_webview/native/aw_metrics_log_uploader.h:15: class AwMetricsLogUploader : public ::metrics::MetricsLogUploader { Nit: Provide a short comment to explain what this does. https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... File android_webview/native/aw_metrics_service_client_impl.cc (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... android_webview/native/aw_metrics_service_client_impl.cc:216: // server_url & mime_type are unused because WebView uses the platform logging Nit: Put ||'s around param names you're referring to from the comment.
uploaded #3 https://codereview.chromium.org/2778203003/diff/20001/android_webview/java/sr... File android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/java/sr... android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java:72: // Takes an uncompressed, serialized UMA proto. On 2017/03/29 19:19:17, Alexei Svitkine (slow) wrote: > Nit: Add a sentence at the beginning to explain what this does - beyond just > documenting the param. I added a bit. This whole class is vague by design, though, since WebView tries to feign ignorance of its downstream components. (I'd normally balk at such a vague and noncommittal name as "PlatformServiceBridge".) https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... File android_webview/native/aw_metrics_log_uploader.cc (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... android_webview/native/aw_metrics_log_uploader.cc:28: compression::GzipUncompress(compressed_log_data, &log_data); On 2017/03/29 19:19:17, Alexei Svitkine (slow) wrote: > I agree about your point about this leaking implementation details from > components/metrics. If you prefer, an option would be to make a wrapper for this > in components/metrics that's called "DecodeLogData" or something like that - and > then this can call it instead of gzip directly. > > (Up to you - can also done in a separate CL). Okay, I'll do that separately. https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... File android_webview/native/aw_metrics_log_uploader.h (right): https://codereview.chromium.org/2778203003/diff/20001/android_webview/native/... android_webview/native/aw_metrics_log_uploader.h:15: class AwMetricsLogUploader : public ::metrics::MetricsLogUploader { On 2017/03/29 19:19:17, Alexei Svitkine (slow) wrote: > Nit: Provide a short comment to explain what this does. Done.
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2778203003/#ps40001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2778203003/#ps60001 (title: "implement RegisterAwMetricsLogUploader")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by paulmiller@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from michaelbai@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/2778203003/#ps80001 (title: "unimplemnet RegisterAwMetricsLogUploader")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490826727566720,
"parent_rev": "23c9664fcc2ffd6bdcffbca5355ba4cd083f7060", "commit_rev":
"a5febff94dd70a488bd13ace5a182a3b3d11156b"}
Message was sent while issue was closed.
Description was changed from ========== WebView: Create UMA uploader WebView uses an unspecified platform mechanism to upload UMA logs, rather than the typical cronet path. This creates a WebView-specific MetricsLogUploader, which sends logs to PlatformServiceBridge, which connects to the platform mechanism. BUG=698047 ========== to ========== WebView: Create UMA uploader WebView uses an unspecified platform mechanism to upload UMA logs, rather than the typical cronet path. This creates a WebView-specific MetricsLogUploader, which sends logs to PlatformServiceBridge, which connects to the platform mechanism. BUG=698047 Review-Url: https://codereview.chromium.org/2778203003 Cr-Commit-Position: refs/heads/master@{#460649} Committed: https://chromium.googlesource.com/chromium/src/+/a5febff94dd70a488bd13ace5a18... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a5febff94dd70a488bd13ace5a18... |
