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

Issue 658300 link

Starred by 4 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Make Java RecordHistogram use CachedHistograms if native hasn't been initialized yet

Project Member Reported by asvitk...@chromium.org, Oct 21 2016

Issue description

Make 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.
 
Project Member

Comment 1 by sheriffbot@chromium.org, Oct 23 2017

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Components: -Internals>Metrics Internals>Metrics>UMA
Status: Available (was: Untriaged)
Still relevant
Labels: -Hotlist-Recharge-Cold
Cc: asvitk...@chromium.org dskiba@chromium.org
 Issue 838308  has been merged into this issue.
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

Comment 6 by dskiba@chromium.org, 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.

@#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.
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.)
@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.
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.
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?
GPU process I think also uses ChildProcessHost, so it should be same as renders.

Sign in to add a comment