NOT FOR REVIEW - Slow Reports Reference Implementation

NOT FOR REVIEW


.

BUG=

[oystein (OOO til 10th of July), 2015-03-16 20:33:04]

oysteine@chromium.org changed reviewers:
+ oysteine@google.com

[oystein (OOO til 10th of July), 2015-03-16 20:33:16]

oysteine@chromium.org changed reviewers:
+ oysteine@chromium.org
- oysteine@google.com

[shatch, 2015-03-17 15:52:53]

On 2015/03/16 20:33:16, Oystein wrote:
> mailto:oysteine@chromium.org changed reviewers:
> + mailto:oysteine@chromium.org
> - mailto:oysteine@google.com

lgtu?

[shatch, 2015-04-23 20:10:12]

simonhatch@chromium.org changed reviewers:
- oysteine@chromium.org

[shatch, 2015-04-28 17:00:23]

https://codereview.chromium.org/1002103004/diff/500001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager.cc (right):

https://codereview.chromium.org/1002103004/diff/500001/content/browser/tracin...
content/browser/tracing/background_tracing_manager.cc:21: std::string
BackgroundTracingManager::ParseOutEndpointContents(const std::string& contents)
const {
Thinking about refactoring all of this out into some sort of parser/factory
class that handles all of this.

[shatch, 2015-05-05 18:50:21]

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
File content/public/browser/background_tracing_manager.h (right):

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:64:
std::vector<MonitoringRule> configs;
Anybody got any thoughts on these configs?

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:108: class UploadSink :
I moved this in here, should we just leave it out in the content namespace as
BackgroundTracingUploadSink?

[oystein (OOO til 10th of July), 2015-05-05 19:28:29]

oysteine@chromium.org changed reviewers:
+ oysteine@chromium.org

[oystein (OOO til 10th of July), 2015-05-05 19:28:31]

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:59: void
BackgroundTracingManagerImpl::WhenIdle(base::Callback<void()> idle_callback) {
Is this used anywhere? What's it for?

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:204:
printf("BackgroundTracingManagerImpl::OnFinalizeComplete\n");
kill

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:207: is_idle_ = true;
Do we need to track this? Can we check if the upload sink is still uploading,
instead of keeping extra state, wherever we currently check this?

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:218: is_gathering_ =
true;
Can we query the TracingController for this instead? Like if it has a trace data
sink set?

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:222: new
TraceDataEndpointWrapper(
Do we delete this anywhere? Can we store the wrapper instead of the
upload_sink_?

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:243: default:
Maybe without the default label, so we know every preset gets handled. Then move
the NOTREACHED() after the switch().

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
File content/public/browser/background_tracing_manager.h (right):

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:32: Mode mode;
private or in the impl maybe?

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:35: void
IntoDict(base::DictionaryValue*);
Are these the to/from JSON things? I don't remember the context.

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:64:
std::vector<MonitoringRule> configs;
On 2015/05/05 18:50:21, shatch wrote:
> Anybody got any thoughts on these configs?

Hmm. Maybe this should just be how it's stored internally, and we just have a
way to add a single new rule in the public API, for each call? I could see these
coming from multiple sources; i.e. Finch could add rules, command-line switches
could add rules...

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:108: class UploadSink :
On 2015/05/05 18:50:21, shatch wrote:
> I moved this in here, should we just leave it out in the content namespace as
> BackgroundTracingUploadSink?

Or maybe even just TracingUploadSink; they aren't really specific to background
tracing.

[shatch, 2015-05-05 20:20:23]

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
File content/browser/tracing/background_tracing_manager_impl.cc (right):

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:59: void
BackgroundTracingManagerImpl::WhenIdle(base::Callback<void()> idle_callback) {
On 2015/05/05 19:28:30, Oystein wrote:
> Is this used anywhere? What's it for?

Used to notify anybody who's interested that the BackgroundTracingManager is
between traces. Can potentially set a new config at this time.

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:204:
printf("BackgroundTracingManagerImpl::OnFinalizeComplete\n");
On 2015/05/05 19:28:30, Oystein wrote:
> kill

Done.

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:207: is_idle_ = true;
On 2015/05/05 19:28:30, Oystein wrote:
> Do we need to track this? Can we check if the upload sink is still uploading,
> instead of keeping extra state, wherever we currently check this?

Added a function on TracingControllerImpl instead.

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:218: is_gathering_ =
true;
On 2015/05/05 19:28:30, Oystein wrote:
> Can we query the TracingController for this instead? Like if it has a trace
data
> sink set?

Nah there's no way to query that. Will have to add it.

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:222: new
TraceDataEndpointWrapper(
On 2015/05/05 19:28:30, Oystein wrote:
> Do we delete this anywhere? Can we store the wrapper instead of the
> upload_sink_?

CreateCompressedStringSink takes a refptr. Uploaded a version that stores the
wrapper locally, it's more code, see if it looks better or not.

https://codereview.chromium.org/1002103004/diff/580001/content/browser/tracin...
content/browser/tracing/background_tracing_manager_impl.cc:243: default:
On 2015/05/05 19:28:30, Oystein wrote:
> Maybe without the default label, so we know every preset gets handled. Then
move
> the NOTREACHED() after the switch().

Done.

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
File content/public/browser/background_tracing_manager.h (right):

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:35: void
IntoDict(base::DictionaryValue*);
On 2015/05/05 19:28:31, Oystein wrote:
> Are these the to/from JSON things? I don't remember the context.

Yep

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:64:
std::vector<MonitoringRule> configs;
On 2015/05/05 19:28:31, Oystein wrote:
> On 2015/05/05 18:50:21, shatch wrote:
> > Anybody got any thoughts on these configs?
> 
> Hmm. Maybe this should just be how it's stored internally, and we just have a
> way to add a single new rule in the public API, for each call? I could see
these
> coming from multiple sources; i.e. Finch could add rules, command-line
switches
> could add rules...

so something like:

class BackgroundTracingPreemptiveConfig {
 public:
  void AddHistogramTriggerRule(...);
  void AddNamedTriggerRule(...);
 private:
  ..stuff
}

Ditto for ReactiveConfig?

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:108: class UploadSink :
On 2015/05/05 19:28:31, Oystein wrote:
> On 2015/05/05 18:50:21, shatch wrote:
> > I moved this in here, should we just leave it out in the content namespace
as
> > BackgroundTracingUploadSink?
> 
> Or maybe even just TracingUploadSink; they aren't really specific to
background
> tracing.

I guess, but who else might use it? Kinda feels we should keep it as
BackgroundTracingUploadSink since that's the pattern here.

[oystein (OOO til 10th of July), 2015-05-05 20:40:10]

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
File content/public/browser/background_tracing_manager.h (right):

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:64:
std::vector<MonitoringRule> configs;
On 2015/05/05 20:20:23, shatch wrote:
> On 2015/05/05 19:28:31, Oystein wrote:
> > On 2015/05/05 18:50:21, shatch wrote:
> > > Anybody got any thoughts on these configs?
> > 
> > Hmm. Maybe this should just be how it's stored internally, and we just have
a
> > way to add a single new rule in the public API, for each call? I could see
> these
> > coming from multiple sources; i.e. Finch could add rules, command-line
> switches
> > could add rules...
> 
> so something like:
> 
> class BackgroundTracingPreemptiveConfig {
>  public:
>   void AddHistogramTriggerRule(...);
>   void AddNamedTriggerRule(...);
>  private:
>   ..stuff
> }
> 
> Ditto for ReactiveConfig?

I was thinking more that the config class which gets passed to AddScenario (or
RegisterScenario or whatever is better) just contains stuff for a single rule,
rather than all of them. I don't think there was any particular reason to just
be able to set the entire thing at once, but maybe I'm forgetting something.

https://codereview.chromium.org/1002103004/diff/580001/content/public/browser...
content/public/browser/background_tracing_manager.h:108: class UploadSink :
On 2015/05/05 20:20:23, shatch wrote:
> On 2015/05/05 19:28:31, Oystein wrote:
> > On 2015/05/05 18:50:21, shatch wrote:
> > > I moved this in here, should we just leave it out in the content namespace
> as
> > > BackgroundTracingUploadSink?
> > 
> > Or maybe even just TracingUploadSink; they aren't really specific to
> background
> > tracing.
> 
> I guess, but who else might use it? Kinda feels we should keep it as
> BackgroundTracingUploadSink since that's the pattern here.

True enough.