New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 695880 link

Starred by 6 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug

Blocking:
issue 706422



Sign in to add a comment

OnDisk persistent metrics can be reported with the wrong field trials

Project Member Reported by asvitk...@chromium.org, Feb 24 2017

Issue description

OnDisk persistent metrics can be reported with the wrong field trials.

See this report:

https://groups.google.com/a/google.com/forum/#!topic/uma-users/tvraijd1Ucw
 
Cc: tbansal@chromium.org
This could be because we have a timing whole on how we capture the previous system profile - it could come from a run that doesn't correspond to the OnDisk persistent histogram file that we're uploading.

To fix this, how about we add the system profile into the memory mapped metrics file - and report them in a separate log with that profile?
That should be easily done.  The allocator can take any kind of record; they'll just be ignored by the histogram system.

Am I correct in understanding that this would have affected all previous "stability" histograms as well?

Comment 4 by siggi@chromium.org, Feb 24 2017

Cc: manzagop@chromium.org
How about we take a small step back and think about the right way to do this? Right now it sounds like we're proposing that the field trials state be stored in no fewer than 3 shared memory segment:
1. The browser->renderer segment.
2. The crumbs.
3. The histogram segment.

This seems - suboptimal - to me?

Also, saving the records isn't too difficult.  Keeping them up-to-date is more so.
So if we don't save them together, then there's a chance things they can get out of sync - since each of those have different lifetimes.

Maybe we can get rid of the one in local state entirely if we put things in the histograms segment. Maybe crumbs could also use the info from the histograms segment?
The histograms segment is currently cleaned up very early during browser startup so that would require some changes.

However, the upcoming "config" segment might be a better choice.  It's what is shared with all sub-processes and will need to be kept around as a reference for the next browser startup anyway.
Blocking: 706422
There is a Q2 OKR to embed the full system profile inside the .pma file and so always properly associate the metrics within.
I'd love to discuss this with whoever takes it on. Esp. wrt crumbs duplication.
Project Member

Comment 11 by bugdroid1@chromium.org, May 30 2017

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

commit 4cde5b7b932ff3ae33107778f592fc21021a63c1
Author: bcwhite <bcwhite@chromium.org>
Date: Tue May 30 15:26:43 2017

Support persistent system profiles.

This allows the entire system profile to be saved into one or more
persistent memory segments for use after the process exits.

The initial version saves only the snapshot known during metrics
reporting but later versions will build upon this to keep the profile
up-to-date in real time.

BUG=695880

Review-Url: https://codereview.chromium.org/2907543003
Cr-Commit-Position: refs/heads/master@{#475534}

[modify] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/base/metrics/persistent_memory_allocator.cc
[modify] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/base/metrics/persistent_memory_allocator.h
[modify] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/chrome/browser/chrome_browser_field_trials.cc
[modify] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/components/metrics/BUILD.gn
[modify] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/components/metrics/metrics_log.cc
[add] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/components/metrics/persistent_system_profile.cc
[add] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/components/metrics/persistent_system_profile.h
[add] https://crrev.com/4cde5b7b932ff3ae33107778f592fc21021a63c1/components/metrics/persistent_system_profile_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Jun 9 2017

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

commit 8e72aa59dfdea6e261b06d8061e488f76801898f
Author: bcwhite <bcwhite@chromium.org>
Date: Fri Jun 09 18:59:18 2017

Persistent metrics files that have an embedded profile don't need to
be sent during startup.  Instead, send them as independent logs
once the browser is already running.

BUG=695880

Review-Url: https://codereview.chromium.org/2918533003
Cr-Commit-Position: refs/heads/master@{#478359}

[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/chrome/browser/metrics/chrome_metrics_service_client.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/file_metrics_provider.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/file_metrics_provider.h
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/file_metrics_provider_unittest.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_log.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_log.h
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_log_store.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_provider.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_provider.h
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_service.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_service.h
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/metrics_service_unittest.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/persistent_system_profile.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/persistent_system_profile.h
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/components/metrics/persistent_system_profile_unittest.cc
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/tools/metrics/histograms/enums.xml
[modify] https://crrev.com/8e72aa59dfdea6e261b06d8061e488f76801898f/tools/metrics/histograms/histograms.xml

Project Member

Comment 13 by bugdroid1@chromium.org, Jun 14 2017

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

commit 8ccb6aa1a42c3e72c4348b80932679b7ac6edb21
Author: bcwhite <bcwhite@chromium.org>
Date: Wed Jun 14 21:45:03 2017

Persist core system profile during startup.

Almost 40% of persistent metrics files are missing the embedded system
profile which is stored about 1 minute into the process lifetime when
the initial metrics upload is sent.

This distinguishes between basic and complete system profiles allowing
the core profile to be stored when the Log is created and be replaced by
a more complete version later.  Thus basic profile will thus be available
even if the browser crashes almost immediately.

BUG=695880

Review-Url: https://codereview.chromium.org/2938013002
Cr-Commit-Position: refs/heads/master@{#479513}

[modify] https://crrev.com/8ccb6aa1a42c3e72c4348b80932679b7ac6edb21/components/metrics/file_metrics_provider_unittest.cc
[modify] https://crrev.com/8ccb6aa1a42c3e72c4348b80932679b7ac6edb21/components/metrics/metrics_log.cc
[modify] https://crrev.com/8ccb6aa1a42c3e72c4348b80932679b7ac6edb21/components/metrics/persistent_system_profile.cc
[modify] https://crrev.com/8ccb6aa1a42c3e72c4348b80932679b7ac6edb21/components/metrics/persistent_system_profile.h
[modify] https://crrev.com/8ccb6aa1a42c3e72c4348b80932679b7ac6edb21/components/metrics/persistent_system_profile_unittest.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 29 2017

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

commit c1ddec34b40c28a89bea7e53eea719e62d6d3df3
Author: bcwhite <bcwhite@chromium.org>
Date: Thu Jun 29 14:13:04 2017

Support add-on 'field trial' records.

Field trials are activated at any time so its important to record
their presence immediately rather than wait for the next "complete"
system profile to be provided.

These are written as "extension" records and otherwise ignored until
recovery of the profile at which point their contents are used to
extend the stored version.

BUG=695880

Review-Url: https://codereview.chromium.org/2950173003
Cr-Commit-Position: refs/heads/master@{#483355}

[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/chrome/browser/metrics/field_trial_synchronizer.cc
[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/components/metrics/persistent_system_profile.cc
[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/components/metrics/persistent_system_profile.h
[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/components/metrics/persistent_system_profile_unittest.cc
[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/components/variations/active_field_trials.cc
[modify] https://crrev.com/c1ddec34b40c28a89bea7e53eea719e62d6d3df3/components/variations/active_field_trials.h

Project Member

Comment 15 by bugdroid1@chromium.org, Jun 30 2017

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

commit 757034c63af4b1c61eeebf070e2123f787be51f7
Author: bcwhite <bcwhite@chromium.org>
Date: Fri Jun 30 17:45:56 2017

Add histograms to learn about metrics files dropped for lack of an embedded profile.

BUG=695880

Review-Url: https://codereview.chromium.org/2966773002
Cr-Commit-Position: refs/heads/master@{#483746}

[modify] https://crrev.com/757034c63af4b1c61eeebf070e2123f787be51f7/components/metrics/file_metrics_provider.cc
[modify] https://crrev.com/757034c63af4b1c61eeebf070e2123f787be51f7/tools/metrics/histograms/histograms.xml

Project Member

Comment 16 by bugdroid1@chromium.org, Jul 4 2017

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

commit 38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6
Author: bcwhite <bcwhite@chromium.org>
Date: Tue Jul 04 15:48:00 2017

Put BrowserMetrics with embedded profiles into subdir for auto-upload.

Because the BrowserMetrics file now has an embedded profile, it isn't
sent as part of the startup sequence but a minute or so after.  That
means the browser could exit and not send anything with the "previous
run" metrics file being overwritten at the next startup.

Instead, append a date-stamp to the last-run filename and move it to
a subdirectory from which files will be removed only after they are
uploaded.

BUG=695880

Review-Url: https://codereview.chromium.org/2938263002
Cr-Commit-Position: refs/heads/master@{#484116}

[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/base/metrics/persistent_histogram_allocator.cc
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/base/metrics/persistent_histogram_allocator.h
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/chrome/browser/chrome_browser_field_trials.cc
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/chrome/browser/metrics/chrome_metrics_service_client.cc
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/components/metrics/file_metrics_provider.cc
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/components/metrics/file_metrics_provider.h
[modify] https://crrev.com/38d8bb9328a2d0a1b7d8373ddd70971c1ce4eeb6/components/metrics/file_metrics_provider_unittest.cc

Status: Fixed (was: Assigned)
Is it possible to know which version this was fixed?
Would you settle for M62 branch point?  It's been active since before M61 went to beta.
So, M61 beta/stable would have this. That's great!! Thank you so much for fixing this.
Status: Assigned (was: Fixed)
Verify this is working for Android.

Sign in to add a comment