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

Issue 602295 link

Starred by 8 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 544971



Sign in to add a comment

Include Persistent Histograms in chrome://histograms

Project Member Reported by bcwh...@chromium.org, Apr 11 2016

Issue description

Version: M51  (with PersistentHistograms experiment enabled)
OS: All

--enable-features=PersistentHistograms

If the PersistentHistograms experiment is active, histograms created by subprocesses (notably the Renderer) will not appear in chrome://histograms.

Metrics reported from sub-processes have never been reliable because they were only transferred during the periodic upload, about every 30 minutes on desktop.

Still, it would be convenient to have them available.

 
Suggestion - a button on the chrome://histograms page to force a sync with the subprocesses? Or perhaps whenever chrome://histograms loads it forces a sync - so to refresh the histograms you reload the page.

Comment 2 by rbyers@chromium.org, Apr 19 2016

Blockedon: 544971
Cc: phil...@opera.com tdres...@chromium.org rbyers@chromium.org
Previously it was every 30 minutes AND on every navigation, right?  In practice I know many blink engineers have had a workflow like:
 - load a page
 - hit reload to ensure the UseCounter and renderer histograms are reported
 - open chrome://histograms/FeatureObserver to see the use counter results for that page

In practice this seems to have been very reliable.  It's unfortunate that  issue 544971  has broken this workflow... 
CL is out for review:
https://codereview.chromium.org/1880803003/

Comment 4 by rbyers@chromium.org, May 30 2016

Status: Started (was: Assigned)
Any update?
There's a big refactoring going on that will affect this (mostly solve it).

FYI:  Subprocess metrics are now merged with all the others, but during metrics collection (every 30 minutes on desktop) or at process exit.

I'm working on a CL to do the merge when chrome://histograms is being displayed.

Right now, the condition of chrome://histograms is back to pre-persistent levels.  All histograms are merged and merged during a collection cycle.  Additionally, they are also merged when a process exits.

Forcing a merge before displaying that page is theoretically possible since the display code is in content/ and all the MetricsService code (which is the only place that knows of all the proivders) is also in content/ or components/.  Thus, there are no "up-call" violations.

However, it turns out that the only access to the global MetricsService object happens to be through a pointer held in chrome/ (the BrowserProcess, to be exact) and accessing that would be an up-call violation.

The choices going forward seem to be:

1) Store the MetricsService pointer somewhere more accessible.
2) Inject a MetricsService pointer into the ChromeProtocolHandler object.
3) Add a new mechanism (in StatisticsRecorder?) to do the merge and have
   all providers also register there.

I'll keep thinking about it but unless there's a new critical need for always-up-to-date histograms, I'm going to put this on hold for now.

content/ shouldn't be calling through into components/metrics (most components shouldn't be used from content), so we would need a delegate function on the content interface for this.
Status: WontFix (was: Started)
Reopen if this becomes important.
Cc: vsu...@chromium.org posciak@chromium.org hsiangc@chromium.org rohi...@chromium.org crouleau@chromium.org ericde@chromium.org bcwh...@chromium.org liber...@chromium.org
 Issue 652793  has been merged into this issue.
Adding some information from  Issue 652793 : 
The following test cases all use histograms and will fail if the results do not appear:

video_ChromeHWDecodeUsed
video_ChromeRTCHWDecodeUsed
video_ChromeVidResChangeHWDecode
video_WebRtcPerf
video_PowerConsumption

These failures occur when running the tests on a dev-built image (tested on dev-built R56-8931 for Lars). However, using a partner image (tested on partner image R56-8932 for Lars) results in a pass. 

Perhaps the metrics collection cycle was set to a very short interval for the ChromeOS browser and the collection cycle for Chromium browsers was left at 30 minutes?

These tests should also be able to run on Chromium images without failing due to histogram metrics not being gathered to validate regressions.
Labels: -Pri-3 M-56 OS-All Pri-2
Status: Available (was: WontFix)
Re-opening this.

Given that multiple people have ran into this issue recently (i.e. both on crbugs and the chromium-dev thread) and the fact that the new persistent metrics system will regress the status quo behavior (i.e. the push mechanism from renderer will no longer work), I think we need to fix this.

Happy to discuss on how we can hook this up. I still feel that we should pull the data as a result of going to chrome://histograms (rather than supporting the push mechanism which I'm hoping we can just get rid of long term).
Status: Assigned (was: Available)
Cc: foolip@chromium.org
Cc: -phil...@opera.com
Cc: ihf@chromium.org warx@chromium.org
Chrome on Chrome OS gardener here. We're seeing a bunch of video suite autotest failures on our pre-flight queue:

video_ChromeHWDecodeUsed
video_ChromeRTCHWDecodeUsed
video_ChromeRTCHWEncodeUsed

With messages like:
ERROR: Media.GpuVideoDecoderInitializeStatus not loaded or histogram bucket not found or histogram bucket found at < 100%, new report]

See Issue 684662 for an example.

These will prevent chrome on chromeos from doing its daily uprev.

I noticed this CL landed recently:

Make MappedFile (aka OnDisk) the default for persistent histograms.
Review-Url: https://codereview.chromium.org/2647353003

I'm not too familiar with the video tests or what's going on with histograms, but could that CL be related?  Could we try reverting it?

That's probably it.  If you test is trying to access histograms in other processes, it will fail when persistence is used.

Have your test call SubprocessMetricsProvider::MergeHistogramDeltasForTesting() to merge in histograms from those other processes before checking the histogram counts.

Cc: jamescook@chromium.org
bcwhite, thanks for the quick reply. Unfortunately Monorail didn't send me email with your comment. :-(

These are not typical chrome-side tests. They are Chrome OS device autotests. I'm not very familiar with them. I think they just start chrome with a video file then use telemetry to read some existing histogram constants. I'm not sure if they run any test-specific code in chrome. I'll ask.

If you're curious here the source of one of the autotests:
https://cs.corp.google.com/chromeos_public/src/third_party/autotest/files/client/site_tests/video_ChromeHWDecodeUsed/video_ChromeHWDecodeUsed.py?type=cs&q=video_ChromeHWDecodeUsed&sq=package:%5Echromeos_(public%7Cinternal)$&l=17

Are you familiar with tests outside of chrome using histograms in this way? Any idea how other people have solved this problem?

achuith, is there a way to get telemetry to call that test method (SubprocessMetricsProvider::MergeHistogramDeltasForTesting()) in the video tests? Or all tests?

bcwhite, I assume that has to be called in the browser process?

Cc: achuith@chromium.org
er, +achuith:

achuith, is there a way to get telemetry to call that test method (SubprocessMetricsProvider::MergeHistogramDeltasForTesting()) in the video tests? Or all tests?

bcwhite, I assume that has to be called in the browser process?

Yes, the browser process needs to call this.  It must already being doing something to fetch histograms from sub-processes, possibly HistogramSubscriber ::FetchHistograms (via RPC) because otherwise this would have failed from the beginning.

We can call a browser C++ method like SubprocessMetricsProvider::MergeHistogramDeltasForTesting() in one of two ways.

The easy way - add a command line flag to chrome, and the video tests can add this flag before they launch chrome, through extra_browser_args:
https://cs.corp.google.com/chromeos_public/src/third_party/autotest/files/client/common_lib/cros/chrome.py?l=77

The limitation here of course is that there is no control over when this gets called, and it sounds like this needs to get called right before histograms are collected at the end of the test.

The complicated way is to add an javascript private extension api that calls this method. Telemetry can then connect to a pre-loaded extension and call this api which will call the C++ method. 
Don't the histograms get written when the browser is shut down? Would it be possible to restart chrome at the end of the video test and then collect histograms?

Comment 24 by warx@chromium.org, Jan 25 2017

"Don't the histograms get written when the browser is shut down?" currently, it is not -> https://cs.corp.google.com/chromeos_public/src/third_party/autotest/files/client/site_tests/video_ChromeHWDecodeUsed/video_ChromeHWDecodeUsed.py?l=86
A clean browser shutdown removes the disk file.  It is expected that metrics were logged in other ways and nothing remains to be uploaded by the next startup.

But even that assumes using a memory-mapped file to back the histogram storage, something that is not necessarily true.

Comment 26 by warx@chromium.org, Jan 25 2017

bcwhite: chromeos autotest histogram_verifier is using the histogram url with pattern as "chrome://histograms/[NameOfHistogram], is this compatible with your code? https://cs.corp.google.com/aosp-master/external/autotest/client/cros/video/histogram_verifier.py?q=histogram_verifier&dr&l=17
Ah, that would be this very issue, then.  No, fetching about://histograms doesn't do a subprocess fetch.

Okay, so I need to address *this* issue before re-landing the "make default" CL so that the verification tests pass.

I was under the impression that previous code also didn't fetch histograms from subprocesses.  Hmmm....

Comment 28 by warx@chromium.org, Jan 26 2017

OK, sounds like it. If you started making changes, wait for landing it until there is a chrome upreved for pfq. I will update here. Thanks!
Issue 685176 has been merged into this issue.
Labels: M-57
Status: Fixed (was: Assigned)
https://codereview.chromium.org/2658163002/ (which is associated with this issue but for some reason did not post to it even though it landed 10 hours ago) adds this feature and is present in build 58.0.2999.0 and forward.

Histograms from subprocesses are now merged with every display of the chrome://histograms page so will always be up-to-date.


Should this be marked M-58 if it landed in a M58 version?
Labels: M-58
Labels: -M-56 -M-57

Sign in to add a comment