Include Persistent Histograms in chrome://histograms |
|||||||||||||||
Issue descriptionVersion: 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.
,
Apr 19 2016
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...
,
Apr 20 2016
CL is out for review: https://codereview.chromium.org/1880803003/
,
May 30 2016
Any update?
,
Jun 3 2016
There's a big refactoring going on that will affect this (mostly solve it).
,
Jun 10 2016
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.
,
Jun 23 2016
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.
,
Jun 28 2016
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.
,
Sep 16 2016
Reopen if this becomes important.
,
Oct 21 2016
Issue 652793 has been merged into this issue.
,
Oct 26 2016
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.
,
Oct 26 2016
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).
,
Oct 26 2016
,
Nov 11 2016
,
Nov 11 2016
,
Jan 24 2017
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?
,
Jan 24 2017
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.
,
Jan 25 2017
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?
,
Jan 25 2017
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?
,
Jan 25 2017
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?
,
Jan 25 2017
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.
,
Jan 25 2017
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.
,
Jan 25 2017
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?
,
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
,
Jan 25 2017
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.
,
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
,
Jan 25 2017
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....
,
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!
,
Jan 26 2017
Issue 685176 has been merged into this issue.
,
Feb 1 2017
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.
,
Feb 1 2017
Should this be marked M-58 if it landed in a M58 version?
,
Feb 1 2017
,
Feb 1 2017
|
|||||||||||||||
►
Sign in to add a comment |
|||||||||||||||
Comment 1 by mikelawther@chromium.org
, Apr 19 2016