Fix for broken cases here https://chromium-review.googlesource.com/c/chromium/src/+/1161162

Fix for int64_t cases I leave to metrics team. Probably it'd be good to redo SourceId as something that can't trivially convert to smaller types.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0190a578443a0dbac2154187b763d1f451a66f54

commit 0190a578443a0dbac2154187b763d1f451a66f54
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Thu Aug 02 21:47:24 2018

Use ukm::SourceId instead of too-small int32_t for UKM reports.

Without this we end up using bogus source ids for reporting to UKM
which end up dropped.

BUG= 870407 
TEST=none

Change-Id: I6dd43a70dad670613c9bffe440a5f4d8934d3a42
Reviewed-on: https://chromium-review.googlesource.com/1161162
Reviewed-by: Justin DeWitt <dewittj@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580337}
[modify] https://crrev.com/0190a578443a0dbac2154187b763d1f451a66f54/components/offline_pages/core/offline_pages_ukm_reporter.cc
[modify] https://crrev.com/0190a578443a0dbac2154187b763d1f451a66f54/media/mojo/services/media_metrics_provider.cc
[modify] https://crrev.com/0190a578443a0dbac2154187b763d1f451a66f54/media/mojo/services/video_decode_perf_history.cc
[modify] https://crrev.com/0190a578443a0dbac2154187b763d1f451a66f54/media/mojo/services/watch_time_recorder.cc

Simple int32_t -> int64_t type narrowing fix for UKM records.

Pls apply appropriate OSs label. Thank you.

This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How is the change looking in canary? Also could you pls justify merge to M69?

Still waiting for UKM data to come in from today's canary, but no crashes or anything like that.

It's a very simple correctness fix to UKM usage by media and offline pages. Without this we drop some percentage of UKM reports.

As an aside, why are we still doing manual review for changes this close to branch cut? I thought that policy was relaxed after it was determined merges are not the primary source of post-branch release latency.

Thank you dalecurtis@, pls update the bug on Monday morning with canary result. 

The NextAction date has arrived: 2018-08-06

Hmm, despite being a technically correct fix, it's not improving the UKM or UMA counts in a significant way, so it doesn't seem like we need to merge this. If anyone from the metrics team wants to challenge that, please speak up.

https://uma.googleplex.com/p/chrome/timeline_v2/?sid=5e5ea9a15279aeaf02eaeec44fb03ac7

I'll double check once we have Sun/Mon data too and we can make the call for sure then.

Sunday data shows no improvement either, so seems this probably isn't worth merging. I'm not sure why the int32_t is sufficient in this case. The ukm::SourceId should have some bits in the upper half of the int64_t:

https://cs.chromium.org/chromium/src/services/metrics/public/cpp/ukm_source_id.cc

We'll see if my fix for  issue 800610  improves things, but that's too large to merge.

Thank you  dalecurtis@.

Rejecting merge to M69 based on comment #11 and #12. 

Looking at the CL again, it looks like these IDs are only in scope for one URL/Event pair, and the bug that drops the high bits changes both. So all of the data would still matching up, it's just that there is potential for collision with other IDs.  The data that's in the high bits are basically some process-scope random bits, so if all of the recording code that was affected by this bug is in the browser process, there shouldn't be any collisions.

The NextAction date has arrived: 2018-08-07

Okay, closing this as fixed.