Make Java RecordHistogram use CachedHistograms if native hasn't been initialized yet |
|||
Issue descriptionMake Java RecordHistogram use CachedHistograms if native hasn't been initialized yet. In https://codereview.chromium.org/2439113002, I've moved the LaunchMetrics' cached histogram implementation to base/. Per chatting with Ted Choc, it would be good to actually make the default behavior of the existing RecordHistogram.java APIs use CachedMetrics if the native side hasn't been initialized yet. So this bug can track the work for that. We can probably unify the two APIs in the process.
,
Oct 23 2017
Still relevant
,
Oct 23 2017
,
Apr 30 2018
,
Apr 30 2018
From the merged bug: This came out of discussion Dmitry and I were having today: Today there's some code in early startup that records metrics by using the CachedMetrics mechanism, where the metrics are kept in Java memory until native is initialized and then recorded. Other instances of code, just check for LibraryLoader.isInitialized() and omit recording metrics in code if native isn't loaded yet. On top of it, cached metrics in child processes (e.g. renderer) don't get recorded at all because the record signal is tied to activity signals. What we should probably have instead is a RecordMetrics abstraction that does the right thing under the hood: - caches in java if native is not yet loaded - uses better signals to learn when it's safe to record the metrics - records native metrics as today when native is loaded
,
Apr 30 2018
Plus we need to make sure CachedMetrics works for services too. Right now you need to be careful not to use CachedMetrics in code that can run in services, because CachedMetrics are never flushed for them.
Also we have several instances (e.g. RecordCastAction, DownloadNotificationUmaHelper) where code does
if (LibraryLoader.isInitialized()) { RecordMetrics... }
i.e. the fact that RecordMetrics uses native code is leaking out, and if native code is not available, the data is simply discarded.
,
Apr 30 2018
@#6 "careful not to...run in services". I thought user metrics were only recorded while Chrome was in the foreground. In java, I thought it was bookended by UmaSessionStats#startNewSession and logAndEndSession (MetricsService OnAppEnterBackground and OnAppEnterForeground). I don't know if that is still the case, whether it applies to a subset of user actions and histograms, etc... I guess I'd first want to know if it is possible to record metrics in the background at all.
,
Apr 30 2018
It's possible to record in the background. They will not be sent until Chrome hits foreground on a next session. Basically, recording locally just updates the stuff in memory. But that is not serialized and sent up unless Chrome is in foreground or on next session when Chrome uploads not-uploaded data from before. The fact that none of this works when running in different context (e.g. services) seems very disturbing. I wonder if we could add asserts about those - I actually don't have a good sense for how much code from Clank runs in services and whether those code paths hit things that record a lot of metrics. Is there someone on Clank that could drive the above? (Probably under a separate bug since it's not exactly quite the same issue as the two APIs being separate which is what this bug is about.)
,
Apr 30 2018
@Ted, Chrome can be in foreground when services are running, e.g. by services I think Dmitry means our renderers for example. So if the startup code logs histograms in the renderer java, these will not get correctly propagated.
,
Apr 30 2018
I think if it's a renderer, then Chrome's histogram synchronization logic should already work assuming it makes it to native. So RecordHistogram API is fine. CachedMetrics API would work if LibraryLoader.isInitialized() is true - but I guess if it's done before that's the case, then we would need something that calls commitCachedMetrics() when native library is loaded. (I don't know if it never happens in renderer?) However, if there are other services that don't use ChildProcessHost then they won't have histograms working. Because the plumbing we have for child process histograms is via C++ & ChildProcessHost.
,
Apr 30 2018
Yep, looks like commitCachedMetrics() call just doesn't happen in the renderer today. We should probably hook it to be off the native initialization is complete signal. I am less concerned about utility services, but hopefully GPU process works?
,
Apr 30 2018
GPU process I think also uses ChildProcessHost, so it should be same as renders. |
|||
►
Sign in to add a comment |
|||
Comment 1 by sheriffbot@chromium.org
, Oct 23 2017Status: Untriaged (was: Available)