Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2976113002:
Add a sampling scheme to ScrollUpdate.Touch UKM (Closed)
|
Created:
3 years, 5 months ago by Navid Zolghadr Modified:
3 years, 5 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org, dtapuska+chromiumwatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a sampling scheme to ScrollUpdate.Touch UKM
There was a lot of data being reported as part of
the Event.ScrollUpdate.Touch UKM. For now we decided
to reduce the volume of the data by a simple sampling
scheme until we find a more permanent solution.
BUG=742363
Review-Url: https://codereview.chromium.org/2976113002
Cr-Commit-Position: refs/heads/master@{#487904}
Committed: https://chromium.googlesource.com/chromium/src/+/bc43f74097a2452f5238c2922b87a84bfd70a528
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove the overhead for using random function #
Total comments: 4
Patch Set 3 : Renaming the variable #Patch Set 4 : Add missing _ #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
nzolghadr@chromium.org changed reviewers: + dtapuska@chromium.org, rkaplow@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
nzolghadr@chromium.org changed reviewers: + tdresser@chromium.org
https://codereview.chromium.org/2976113002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2976113002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422: if (metric_sampling_ && base::RandUint64() % 10) RandUint64 is thread safe and cryptographically secure. I think we could use something a bit cheaper here. Maybe even just record every nth value?
lgtm
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal https://codereview.chromium.org/2976113002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2976113002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:422: if (metric_sampling_ && base::RandUint64() % 10) On 2017/07/17 16:26:40, tdresser wrote: > RandUint64 is thread safe and cryptographically secure. I think we could use > something a bit cheaper here. Maybe even just record every nth value? I changed the implementation to only use random once at the beginning to also avoid any bias towards any particular cycle.
https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:432: metric_sampling_countdown_ = 0; This line is useless.
On 2017/07/17 20:21:41, dtapuska wrote: > https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:432: > metric_sampling_countdown_ = 0; > This line is useless. lgtm
LGTM % naming nit. https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:428: metric_sampling_countdown_++; "countdown" implies it counts down, but it counts up! I'm not sure what the best name would be though. metric_sampling_events_since_last_sample ? Any better ideas?
On 2017/07/17 20:25:22, tdresser wrote: > LGTM % naming nit. > > https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... > File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc > (right): > > https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... > content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:428: > metric_sampling_countdown_++; > "countdown" implies it counts down, but it counts up! > > I'm not sure what the best name would be though. > metric_sampling_events_since_last_sample ? > > Any better ideas? Issue 739169 covers better client side aggregation.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nzolghadr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... File content/browser/renderer_host/input/render_widget_host_latency_tracker.cc (right): https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:428: metric_sampling_countdown_++; On 2017/07/17 20:25:22, tdresser wrote: > "countdown" implies it counts down, but it counts up! > > I'm not sure what the best name would be though. > metric_sampling_events_since_last_sample ? > > Any better ideas? Done. https://codereview.chromium.org/2976113002/diff/20001/content/browser/rendere... content/browser/renderer_host/input/render_widget_host_latency_tracker.cc:432: metric_sampling_countdown_ = 0; On 2017/07/17 20:21:41, dtapuska wrote: > This line is useless. Done.
The CQ bit was unchecked by nzolghadr@chromium.org
The CQ bit was checked by nzolghadr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org, dtapuska@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/2976113002/#ps60001 (title: "Add missing _")
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": 60001, "attempt_start_ts": 1500479875378350,
"parent_rev": "33133e5767a9c1ca1f7e9defad2d7b3347065589", "commit_rev":
"bc43f74097a2452f5238c2922b87a84bfd70a528"}
Message was sent while issue was closed.
Description was changed from ========== Add a sampling scheme to ScrollUpdate.Touch UKM There was a lot of data being reported as part of the Event.ScrollUpdate.Touch UKM. For now we decided to reduce the volume of the data by a simple sampling scheme until we find a more permanent solution. BUG=742363 ========== to ========== Add a sampling scheme to ScrollUpdate.Touch UKM There was a lot of data being reported as part of the Event.ScrollUpdate.Touch UKM. For now we decided to reduce the volume of the data by a simple sampling scheme until we find a more permanent solution. BUG=742363 Review-Url: https://codereview.chromium.org/2976113002 Cr-Commit-Position: refs/heads/master@{#487904} Committed: https://chromium.googlesource.com/chromium/src/+/bc43f74097a2452f5238c2922b87... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/bc43f74097a2452f5238c2922b87... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
