Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1155)

Issue 306023010: Introduce ProfilerMetricsProvider (Closed)

Created:
6 years, 6 months ago by blundell
Modified:
6 years, 1 month ago
CC:
chromium-reviews, Ilya Sherman, asvitkine+watch_chromium.org, jar+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Introduce ProfilerMetricsProvider This CL introduces ProfilerMetricsProvider, which takes the profiler data recording responsibilities that were previously part of MetricsLog. ProfilerMetricsProvider records profiler data into a cached proto and then merges this data into the actual UMA proto in RecordGeneralMetrics(). BUG=379881 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274528

Patch Set 1 #

Patch Set 2 : Avoid data bleeding between different logs #

Total comments: 2

Patch Set 3 : Comments #

Patch Set 4 : Android build fix #

Total comments: 17

Patch Set 5 : Response to review #

Total comments: 10

Patch Set 6 : Response to review #

Patch Set 7 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+388 lines, -309 lines) Patch
M chrome/browser/metrics/metrics_log.h View 1 chunk +0 lines, -6 lines 0 comments Download
M chrome/browser/metrics/metrics_log.cc View 1 2 3 4 5 chunks +0 lines, -120 lines 0 comments Download
M chrome/browser/metrics/metrics_log_unittest.cc View 4 chunks +0 lines, -175 lines 0 comments Download
M chrome/browser/metrics/metrics_service.h View 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/metrics/metrics_service.cc View 1 2 3 4 5 3 chunks +12 lines, -8 lines 0 comments Download
A chrome/browser/metrics/profiler_metrics_provider.h View 1 2 3 4 1 chunk +45 lines, -0 lines 0 comments Download
A chrome/browser/metrics/profiler_metrics_provider.cc View 1 2 3 4 5 1 chunk +144 lines, -0 lines 0 comments Download
A chrome/browser/metrics/profiler_metrics_provider_unittest.cc View 1 2 3 4 1 chunk +181 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 13 (0 generated)
blundell
Please look carefully at the code (and comments) in ProfileMetricsProvider::ProvideGeneralMetrics(). https://codereview.chromium.org/306023010/diff/20001/chrome/browser/metrics/metrics_service.cc File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/306023010/diff/20001/chrome/browser/metrics/metrics_service.cc#newcode749 ...
6 years, 6 months ago (2014-06-01 16:49:25 UTC) #1
Alexei Svitkine (slow)
Thanks for working on this! Can you file a crbug for this as well? https://codereview.chromium.org/306023010/diff/20001/chrome/browser/metrics/metrics_service.cc ...
6 years, 6 months ago (2014-06-02 15:17:05 UTC) #2
Alexei Svitkine (slow)
https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_log.cc File chrome/browser/metrics/metrics_log.cc (left): https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_log.cc#oldcode18 chrome/browser/metrics/metrics_log.cc:18: #include "base/profiler/alternate_timer.h" Can this be removed? https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_log.cc#oldcode24 chrome/browser/metrics/metrics_log.cc:24: #include ...
6 years, 6 months ago (2014-06-02 17:30:54 UTC) #3
blundell
Thanks! https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_log.cc File chrome/browser/metrics/metrics_log.cc (left): https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_log.cc#oldcode18 chrome/browser/metrics/metrics_log.cc:18: #include "base/profiler/alternate_timer.h" On 2014/06/02 17:30:54, Alexei Svitkine wrote: ...
6 years, 6 months ago (2014-06-02 19:56:43 UTC) #4
Alexei Svitkine (slow)
LGTM % comments Thanks! https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_service.cc File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/306023010/diff/80001/chrome/browser/metrics/metrics_service.cc#newcode737 chrome/browser/metrics/metrics_service.cc:737: void MetricsService::ReceivedProfilerData( On 2014/06/02 19:56:44, ...
6 years, 6 months ago (2014-06-02 20:25:01 UTC) #5
Ilya Sherman
https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/profiler_metrics_provider.h File chrome/browser/metrics/profiler_metrics_provider.h (right): https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/profiler_metrics_provider.h#newcode40 chrome/browser/metrics/profiler_metrics_provider.h:40: bool has_profiler_data_; Do we need this extra variable, or ...
6 years, 6 months ago (2014-06-02 21:58:15 UTC) #6
blundell
https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/metrics_service.cc File chrome/browser/metrics/metrics_service.cc (right): https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/metrics_service.cc#newcode750 chrome/browser/metrics/metrics_service.cc:750: DCHECK(!initial_metrics_log_.get()); On 2014/06/02 20:25:02, Alexei Svitkine wrote: > Can ...
6 years, 6 months ago (2014-06-03 12:23:47 UTC) #7
blundell
The CQ bit was checked by blundell@chromium.org
6 years, 6 months ago (2014-06-03 12:23:53 UTC) #8
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/blundell@chromium.org/306023010/130011
6 years, 6 months ago (2014-06-03 12:24:19 UTC) #9
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium ...
6 years, 6 months ago (2014-06-03 14:21:41 UTC) #10
commit-bot: I haz the power
Change committed as 274528
6 years, 6 months ago (2014-06-03 14:58:28 UTC) #11
Ilya Sherman
https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/profiler_metrics_provider.h File chrome/browser/metrics/profiler_metrics_provider.h (right): https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/profiler_metrics_provider.h#newcode40 chrome/browser/metrics/profiler_metrics_provider.h:40: bool has_profiler_data_; You could, for example, check whether profiler_event_cache_->tracked_object_size() ...
6 years, 6 months ago (2014-06-03 19:02:29 UTC) #12
blundell
6 years, 1 month ago (2014-10-29 13:24:04 UTC) #13
Message was sent while issue was closed.
https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/...
File chrome/browser/metrics/profiler_metrics_provider.h (right):

https://codereview.chromium.org/306023010/diff/100001/chrome/browser/metrics/...
chrome/browser/metrics/profiler_metrics_provider.h:40: bool has_profiler_data_;
On 2014/06/03 19:02:29, Ilya Sherman wrote:
> You could, for example, check whether
> profiler_event_cache_->tracked_object_size() is 0.  I would indeed prefer not
to
> have redundant state tracking if we don't need it -- that's just asking for
> trouble due to the variables getting out of sync with each other.  Thanks!
> 
> On 2014/06/03 12:23:47, blundell wrote:
> > I don't actually know how to do that (I'm not very familiar with protobufs).
> I'm
> > going to send this through CQ, happy to fix in followup.
> > 
> > On 2014/06/02 21:58:16, Ilya Sherman wrote:
> > > Do we need this extra variable, or can we just check whether the cache is
> > empty?
> > 
> 

Sorry, I've never been able to get around to this. To make sure it didn't get
lost, I filed a bug: crbug.com/428316

Powered by Google App Engine
This is Rietveld 408576698