Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1089253003:
Re-land first pass BackgroundTracingManager. (Closed)
|
Created:
5 years, 8 months ago by shatch Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkeleton for BackgroundTracingManager.
This version mostly just directs the TracingController using the specified BackgroundTracingConfig and pushes the compressed trace out to the BackgroundTracingUploadSink.
Specifically, we implement the PREEMPTIVE_TRACING_MODE for the rule MONITOR_AND_DUMP_WHEN_TRIGGER_NAMED, which should allow us to get an experiment going on desktop with a simple trigger and upload.
We can then follow up with additional CL's implementing the rest of the functionality from the clientside doc below.
Needs to land first: https://codereview.chromium.org/1088673003/
Slow Reports Clientside: https://docs.google.com/document/d/1qZmXmodxOKmsTRO27z2WlH2h9Kpf-kjV-k-1pJIogIE/edit?pli=1
go/slow-reports
Committed: https://crrev.com/c9cd43c8963301a6b1ebfb74c4eb03b8c733e9f9
Cr-Commit-Position: refs/heads/master@{#330942}
Committed: https://crrev.com/0b016196499e8b5da3f20d90b201d90339ec88c8
Cr-Commit-Position: refs/heads/master@{#331141}
Patch Set 1 : #Patch Set 2 : Added tests. #Patch Set 3 : More tests. #
Total comments: 11
Patch Set 4 : Fixes. #Patch Set 5 : Fixes. #Patch Set 6 : Ready for review. #Patch Set 7 : #
Total comments: 8
Patch Set 8 : New interface. #
Total comments: 2
Patch Set 9 : Split into multiple files in content/public/browser. #
Total comments: 8
Patch Set 10 : Changes from review. #Patch Set 11 : Split config into separate headers and build fix. #
Total comments: 16
Patch Set 12 : Review changes. #
Total comments: 8
Patch Set 13 : Removed upload_sink. #
Total comments: 9
Patch Set 14 : Changes from review. #Patch Set 15 : Rebase. #
Total comments: 2
Patch Set 16 : Scoped_ptr #Messages
Total messages: 69 (29 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
oysteine@chromium.org changed reviewers: + oysteine@chromium.org
https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_endpoint.cc (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_endpoint.cc:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_endpoint.cc:6: #include "base/logging.h" needed? https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_endpoint.cc:9: #include "third_party/zlib/zlib.h" needed? https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_endpoint.h (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_endpoint.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: remove the (c) https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_endpoint.h:9: #include "base/callback.h" nit: duplicate line https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_manager.cc (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager.cc:12: BackgroundTracingManager::BackgroundTracingManager() : weak_ptr_factory_(this) { is_recording_ isn't being initialized looks like? Also, in an initialization list instead maybe. https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. nit: no (c) https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_browsertest.cc (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:17: #include "content/shell/browser/shell_resource_dispatcher_host_delegate.h" Are all of these includes used here? https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:149: printf("trying\n"); nit: remove https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager_browsertest.cc:154: printf("checking\n"); nit: same https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_scenario.cc (right): https://codereview.chromium.org/1089253003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_scenario.cc:23: base::trace_event::CategoryFilter BackgroundTracingScenario::GetCategoryFilter() nit: maybe base::trace_event::CategoryFilter on a separate line instead.
simonhatch@chromium.org changed reviewers: + dsinclair@chromium.org, nduca@chromium.org
I think this is ready for review, ptal!
On 2015/04/22 16:28:47, shatch wrote: > I think this is ready for review, ptal! ping
https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... File content/browser/tracing/background_tracing_manager.cc (right): https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.cc:30: is_recording_ = true; I don't see this getting reset to false anywhere? https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.cc:33: base::trace_event::TraceOptions(base::trace_event::RECORD_CONTINUOUSLY), Do we always want to set RECORD_CONTINUOUSLY? Won't clients potentially also want UNTIL_FULL or AS_MUCH_AS_POSSIBLE? https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.cc:62: void BackgroundTracingManager::TryFinalizingBackgroundTracing( I'm a little confused by the terminology is the flow along the lines of: TryFinalizingBackgroundTracing -> BeginFinalizingBackgroundTracing -> DisableRecording -> OnFinalizeComplete ->EnableRecording So, we disable and then enable recording? Is it that we want to send a snapshot of a recording and then start another? https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... File content/browser/tracing/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.h:31: BackgroundTracingManager(); If this is a singleton, should these not be private? https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.h:53: bool is_recording_; nit: Can these have a : 1? https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_manager.h:57: friend struct DefaultSingletonTraits<BackgroundTracingManager>; nit: for some reason, I think these typlically come at the start of the private section .... or I may just be making that up. https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... File content/browser/tracing/background_tracing_scenario.h (right): https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_scenario.h:19: BackgroundTracingScenario(scoped_refptr<BackgroundTracingEndpoint>); nit: explicit https://codereview.chromium.org/1089253003/diff/170001/content/browser/tracin... content/browser/tracing/background_tracing_scenario.h:31: }; Do these get copied or does this need a DISALLOW_COPY_AND_ASSIGN?
Patchset #8 (id:190001) has been deleted
After discussion with Nat/Oystein offline, we've settled on a new interface for the BackgroundTracingManager. ptal!
Patchset #8 (id:210001) has been deleted
lgtm to me, a minor question but go for it :) https://codereview.chromium.org/1089253003/diff/230001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/230001/content/public/browser... content/public/browser/background_tracing_manager.h:121: class BackgroundTracingUploadSink should we do one file for the configs and one for the sink? is there convention in public/ for this?
simonhatch@chromium.org changed reviewers: + sievers@chromium.org
+sievers for content/public/browser and the .gypi files. ptal! https://codereview.chromium.org/1089253003/diff/230001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/230001/content/public/browser... content/public/browser/background_tracing_manager.h:121: class BackgroundTracingUploadSink On 2015/05/11 18:23:27, nduca wrote: > should we do one file for the configs and one for the sink? is there convention > in public/ for this? Ah yeah checking https://www.chromium.org/developers/content-module/content-api and looking at content/public/browser/download_manager.h, looks like the convention is to do one file each.
https://codereview.chromium.org/1089253003/diff/250001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1089253003/diff/250001/content/content_browse... content/content_browser.gypi:73: 'public/browser/background_tracing_config.cc', nit: order https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_config.cc (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_config.cc:10: } Hmm that's a lot of implementation, esp. with the TODOs. ('content/public should contain only interfaces, structs and (rarely) static functions') https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_config.h:20: enum Mode { each interface/struct/enum should be in a separate file (https://www.chromium.org/developers/content-module/content-api) https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_manager.h:11: #include "content/public/browser/background_tracing_config.h" nit: I think you can fwd-declare some of these
Patchset #10 (id:270001) has been deleted
https://codereview.chromium.org/1089253003/diff/250001/content/content_browse... File content/content_browser.gypi (right): https://codereview.chromium.org/1089253003/diff/250001/content/content_browse... content/content_browser.gypi:73: 'public/browser/background_tracing_config.cc', On 2015/05/11 21:13:27, sievers wrote: > nit: order Done. https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_config.cc (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_config.cc:10: } On 2015/05/11 21:13:27, sievers wrote: > Hmm that's a lot of implementation, esp. with the TODOs. > ('content/public should contain only interfaces, structs and (rarely) static > functions') Gotcha, looked around at some of the other structs in content/public and followed their example. The Add* helpers here aren't really necessary, and I moved the To/From dict into a static with implementation in the impl file (was following the pattern in content/public/browser/host_zoom_map.h). See if this is ok, otherwise I can go with the other way we talked about (abstract interface for config). https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_config.h:20: enum Mode { On 2015/05/11 21:13:27, sievers wrote: > each interface/struct/enum should be in a separate file > (https://www.chromium.org/developers/content-module/content-api) Yeah I saw that rule but wasn't clear if that also extended to enums/structs declared in classes. A quick search through content/public found about 16 instances of enums declared in classes, so that rule didn't seem to apply here, ie: content/public/browser/navigation_controller.h content/public/browser/power_save_blocker.h I don't mind doing it but moving each enum and struct to a separate file will add 8 new files to this CL. Do you still want these moved or is this ok? https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/250001/content/public/browser... content/public/browser/background_tracing_manager.h:11: #include "content/public/browser/background_tracing_config.h" On 2015/05/11 21:13:27, sievers wrote: > nit: I think you can fwd-declare some of these Done.
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org Link to the patchset: https://codereview.chromium.org/1089253003/#ps290001 (title: "Changes from review.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/290001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org Link to the patchset: https://codereview.chromium.org/1089253003/#ps310001 (title: "Split config into separate headers and build fix.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/310001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry for the lengthy feedback. This on-demand tracing stuff is really cool. I think it's worth overthinking the API stuff just a bit so that even dummies like me understand how to use it :) And so we don't have to go back and tweak it back and forth. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_config.h:21: : public base::RefCountedThreadSafe<BackgroundTracingConfig> { Why RefCounted? Seems overkill for a simple config struct. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_config.h:30: Do you think we could get rid of the config base class and move the fields and functionality from here into the manager interface or somewhere else? The two tracing modes are fundamentally very different, and I think this adds confusion. Unless, you maybe want to specify this differently: You have events (named manual, startup complete, histogram update) and actions: finalize trace, enable tracing if needed. For finalizing the trace for the latter, the enable action could either have a timeout argument for finalizing (if that is the only way to use it you care about), or you could also support specifying other events for the finalizing. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_manager.h:25: virtual bool SetActiveScenario( Question: Do you think it would ever be useful to allow multiple clients (or rather active scenarios) at the same time? I'm asking because the behavior is undefined here for the callers - and there might be multiple that don't know about each other - if there is already an active scenario. Either it needs to clobber the old one, or it needs to fail. At the same time, it doesn't seem too complicated to support multiple callers. You really only care about whether tracing needs to be enabled (someone is waiting to finalize a trace *prior* to the event) and what categories need to be enabled (either it's always the superset or you need to filter depending on the sink). I don't feel strongly, but I think you should at least document here what the behavior is if another scenario is already active. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_manager.h:40: virtual void DidTriggerHappen(TriggerHandle, nit: Maybe avoid the 'Did' prefix here. You are really triggering a trigger :) And maybe 'event' is a better word for trigger, i.e. TriggerNamedEvent()? Or if you want to stick with 'trigger' maybe InvokeNamedTrigger() or DoTrigger() or so? https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_preemptive_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_preemptive_config.h:23: MONITOR_AND_DUMP_WHEN_BROWSER_STARTED, nit: STARTED -> STARTUP_COMPLETE? https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_preemptive_config.h:35: NamedTriggerInfo named_trigger_info; Ok now here subclassing would make sense since you have the union of mutually exclusive stuff. Or what would also work is making a class and hiding the mutually-exclusive fields as members, but having multiple factories for the corresponding type of trigger. Based on the other comments you could also make this part of the manager API instead by having multiple interfaces there. You should also consider for the future if the 'trace *after* an event happened' would also eventually want to share internal events with the 'trace *before* an event happens' category (such as the histogram based one) or vice versa (like 'navigation' you mentioned). The way I look at this API is that you really just want to be able to say 'upload me a trace of a few seconds either before or after a list of certain events' somehow. I'm afraid that going down this route is slightly complicated to follow, and will become more complicated if you extend it later, and might create ill-defined use cases. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_reactive_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:22: enum RuleType { TRACE_ON_TRIGGER_UNTIL_10S_OR_NEXT_TRIGGER_OR_FULL }; nit: so this is TRACE_ON_MANUAL_TRIGGER_...? https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:26: CategoryPreset category_preset; Aha, that's interesting that you allow multiple categories in this mode. Why not in the other mode then?
Btw, is this too simplistic?
struct TracingTrigger {
EventDescription event;
bool trace_before_or_after;
TimeDelta trace_length; // or could be a global setting
}
On 2015/05/14 21:56:31, sievers wrote:
> Btw, is this too simplistic?
>
> struct TracingTrigger {
> EventDescription event;
> bool trace_before_or_after;
> TimeDelta trace_length; // or could be a global setting
> }
Hmm I'm not sure this is much simpler. I'm guessing we'd pass a list of these to
the manager, and you'd have to make sure that trace_before_or_after was set the
same for everything. If it was in preemptive mode, you'd need to also make sure
that any categories in EventDescription match, otherwise it's an invalid config,
and trace_length would only have meaning in reactive mode.
Thanks for taking the time to give detailed feedback Daniel! https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_config.h:21: : public base::RefCountedThreadSafe<BackgroundTracingConfig> { On 2015/05/14 21:25:16, sievers wrote: > Why RefCounted? Seems overkill for a simple config struct. Done. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_config.h:30: On 2015/05/14 21:25:16, sievers wrote: > Do you think we could get rid of the config base class and move the fields and > functionality from here into the manager interface or somewhere else? > > The two tracing modes are fundamentally very different, and I think this adds > confusion. > > Unless, you maybe want to specify this differently: > You have events (named manual, startup complete, histogram update) and actions: > finalize trace, enable tracing if needed. For finalizing the trace for the > latter, the enable action could either have a timeout argument for finalizing > (if that is the only way to use it you care about), or you could also support > specifying other events for the finalizing. When we talked offline, we kicked around the idea of just separating these two configs without a base class. We could do that, and just move the Preemptive/Reactive configs into the manager interface, like: class BackgroundTracingManager { public: struct PreemptiveConfig { ... }; struct ReactiveConfig { ... }; bool StartInPremptiveMode(const PreemptiveConfig&, scoped_refptr<BackgroundTracingUploadSink>); bool StartInReactiveMode(const ReactiveConfig&, scoped_refptr<BackgroundTracingUploadSink>); }; Also renamed the "SetActiveScenario" to "StartIn<X>Mode", feels like this is a lot clearer what's happening? wdyt? https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_manager.h:25: virtual bool SetActiveScenario( On 2015/05/14 21:25:16, sievers wrote: > Question: Do you think it would ever be useful to allow multiple clients (or > rather active scenarios) at the same time? > I'm asking because the behavior is undefined here for the callers - and there > might be multiple that don't know about each other - if there is already an > active scenario. > > Either it needs to clobber the old one, or it needs to fail. > > At the same time, it doesn't seem too complicated to support multiple callers. > You really only care about whether tracing needs to be enabled (someone is > waiting to finalize a trace *prior* to the event) and what categories need to be > enabled (either it's always the superset or you need to filter depending on the > sink). > > I don't feel strongly, but I think you should at least document here what the > behavior is if another scenario is already active. Hmm I'm not sure how easy or useful it would be to support multiple sets of configs. Maybe reactive mode wouldn't be too bad since you'd just need to merge in the new rules. Preemptive mode is more complex in that you'd need to either do as you suggest and do a superset of categories + filtering, or simply fail if the configs can't be combined. I'm not sure there's an easy way to "filter" the finalized trace at the moment, and adding it could be a lot of work. I imagine we'd need to decompress the finalized trace, parse it to filter out the categories, and then recompress it before uploading. I think I'd prefer to go the simple route at the moment and do what TracingController does, which is support just 1 caller at a time and fail if you call it again while it's still working. I'll document it a bit more to make it more clear what the behaviour will be if there's already a config set. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_manager.h:40: virtual void DidTriggerHappen(TriggerHandle, On 2015/05/14 21:25:16, sievers wrote: > nit: Maybe avoid the 'Did' prefix here. You are really triggering a trigger :) > And maybe 'event' is a better word for trigger, i.e. TriggerNamedEvent()? Or if > you want to stick with 'trigger' maybe InvokeNamedTrigger() or DoTrigger() or > so? Done. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_preemptive_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_preemptive_config.h:23: MONITOR_AND_DUMP_WHEN_BROWSER_STARTED, On 2015/05/14 21:25:17, sievers wrote: > nit: STARTED -> STARTUP_COMPLETE? Done. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_preemptive_config.h:35: NamedTriggerInfo named_trigger_info; On 2015/05/14 21:25:17, sievers wrote: > Ok now here subclassing would make sense since you have the union of mutually > exclusive stuff. > Or what would also work is making a class and hiding the mutually-exclusive > fields as members, but having multiple factories for the corresponding type of > trigger. > Hrm, not quite following. Are you suggesting something along the lines of: class BackgroundTracingManager { struct PreemptiveConfig { virtual void AddNamedTriggerRule(...); virtual void AddUMAHistogramTriggerRule(...); }; }; and then defining those in something like PreemptiveConfigImpl? > Based on the other comments you could also make this part of the manager API > instead by having multiple interfaces there. > > You should also consider for the future if the 'trace *after* an event happened' > would also eventually want to share internal events with the 'trace *before* an > event happens' category (such as the histogram based one) or vice versa (like > 'navigation' you mentioned). > > The way I look at this API is that you really just want to be able to say > 'upload me a trace of a few seconds either before or after a list of certain > events' somehow. I'm afraid that going down this route is slightly complicated > to follow, and will become more complicated if you extend it later, and might > create ill-defined use cases. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... File content/public/browser/background_tracing_reactive_config.h (right): https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:22: enum RuleType { TRACE_ON_TRIGGER_UNTIL_10S_OR_NEXT_TRIGGER_OR_FULL }; On 2015/05/14 21:25:17, sievers wrote: > nit: so this is TRACE_ON_MANUAL_TRIGGER_...? Done. https://codereview.chromium.org/1089253003/diff/310001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:26: CategoryPreset category_preset; On 2015/05/14 21:25:17, sievers wrote: > Aha, that's interesting that you allow multiple categories in this mode. > Why not in the other mode then? This mode (reactive) doesn't start tracing until an event triggers it, so you can wait and then get the categories for that event. The other mode has tracing enabled already, which means you had to set the categories ahead of time.
I'll be happy to stamp of this is the route you guys want to take. It just seems a bit complicated to me. Don't you just really need methods to 1) start tracing (i.e. proactively, allowing this to be turned on will give you tracedata right before an event) 2) specify events that trigger a trace (you either get a few seconds before and after or just after depending on whether tracing was on) And *maybe* 3) specify events that do 1)
On 2015/05/15 22:26:03, sievers wrote: > I'll be happy to stamp of this is the route you guys want to take. > It just seems a bit complicated to me. > > Don't you just really need methods to > > 1) start tracing (i.e. proactively, allowing this to be turned on will give you > tracedata right before an event) > 2) specify events that trigger a trace (you either get a few seconds before and > after or just after depending on whether tracing was on) > > And *maybe* 3) specify events that do 1) I don't mind discussing and iterating on this a bit more, there's still some complexity but overall I'm pretty happy with how this has come along with the newer version. If you don't have any strong feelings on this, I'd prefer to move forward with it.
https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_config.cc (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_config.cc:25: BackgroundTracingPreemptiveConfig::~BackgroundTracingPreemptiveConfig() { these are still here in the wrong .cc file https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_config.cc:31: same here https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_reactive_config.h (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:5: #ifndef CONTENT_PUBLIC_BROWSER_BACKGROUND_REACTIVE_TRACING_CONFIG_H_ nit: doesn't match the filename BACKGROUND_REACTIVE_TRACING -> BACKGROUND_TRACING_REACTIVE https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_upload_sink.h (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_upload_sink.h:18: : public base::RefCountedThreadSafe<BackgroundTracingUploadSink> { So this is for the embedder to implement? Being RefCounted raises at least two questions: - what thread will it be unref'd (and potentially be deleted) on? - when will it be deleted? It actually looks like with the current implementation, it might leak.
https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_config.cc (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_config.cc:25: BackgroundTracingPreemptiveConfig::~BackgroundTracingPreemptiveConfig() { On 2015/05/19 19:52:21, sievers wrote: > these are still here in the wrong .cc file Done. https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_config.cc:31: On 2015/05/19 19:52:22, sievers wrote: > same here Done. https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_reactive_config.h (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_reactive_config.h:5: #ifndef CONTENT_PUBLIC_BROWSER_BACKGROUND_REACTIVE_TRACING_CONFIG_H_ On 2015/05/19 19:52:22, sievers wrote: > nit: doesn't match the filename BACKGROUND_REACTIVE_TRACING -> > BACKGROUND_TRACING_REACTIVE Done. https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... File content/public/browser/background_tracing_upload_sink.h (right): https://codereview.chromium.org/1089253003/diff/330001/content/public/browser... content/public/browser/background_tracing_upload_sink.h:18: : public base::RefCountedThreadSafe<BackgroundTracingUploadSink> { On 2015/05/19 19:52:22, sievers wrote: > So this is for the embedder to implement? > Being RefCounted raises at least two questions: > - what thread will it be unref'd (and potentially be deleted) on? > - when will it be deleted? > > It actually looks like with the current implementation, it might leak. Spoke offline, liked your suggestion to just boil this down to a callback instead of passing around this endpoint, so went with that.
https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:24: typedef base::Callback<void(base::RefCountedString*, base::Callback<void()>)> const base::RefCountedString*? https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:24: typedef base::Callback<void(base::RefCountedString*, base::Callback<void()>)> Can you document what the arguments are? Esp. for the other callback it's unclear. https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:24: typedef base::Callback<void(base::RefCountedString*, base::Callback<void()>)> nit: base::Callback<void()> -> base::Closure https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:58: TriggerHandle, nit: 'TriggerHandle trigger_handle' for consistency https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:66: virtual void GetTriggerNameList(std::vector<std::string>& trigger_names) = 0; nit: 'std::vector<std::string>*', by pointer when modified by callee per style guide
https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... File content/public/browser/background_tracing_manager.h (right): https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:24: typedef base::Callback<void(base::RefCountedString*, base::Callback<void()>)> On 2015/05/20 19:22:39, sievers wrote: > nit: base::Callback<void()> -> base::Closure Done. https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:24: typedef base::Callback<void(base::RefCountedString*, base::Callback<void()>)> On 2015/05/20 19:22:40, sievers wrote: > Can you document what the arguments are? > Esp. for the other callback it's unclear. Done. https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:58: TriggerHandle, On 2015/05/20 19:22:39, sievers wrote: > nit: 'TriggerHandle trigger_handle' for consistency Done. https://codereview.chromium.org/1089253003/diff/350001/content/public/browser... content/public/browser/background_tracing_manager.h:66: virtual void GetTriggerNameList(std::vector<std::string>& trigger_names) = 0; On 2015/05/20 19:22:39, sievers wrote: > nit: 'std::vector<std::string>*', by pointer when modified by callee per style > guide Done.
lgtm
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org Link to the patchset: https://codereview.chromium.org/1089253003/#ps370001 (title: "Changes from review.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/370001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #16 (id:410001) has been deleted
Patchset #15 (id:390001) has been deleted
Patchset #15 (id:430001) has been deleted
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1089253003/#ps450001 (title: "Rebase.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/450001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/450001
Message was sent while issue was closed.
Committed patchset #15 (id:450001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/c9cd43c8963301a6b1ebfb74c4eb03b8c733e9f9 Cr-Commit-Position: refs/heads/master@{#330942}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:450001) has been created in https://codereview.chromium.org/1147383002/ by simonhatch@chromium.org. The reason for reverting is: Failed TSan Tests. http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Tests....
Message was sent while issue was closed.
https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... content/public/browser/background_tracing_config.h:34: static void IntoDict(const BackgroundTracingConfig* config, drive by: bit puzzled why this is pass-in instead of returns scoped ptr...
Message was sent while issue was closed.
https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... File content/public/browser/background_tracing_config.h (right): https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... content/public/browser/background_tracing_config.h:34: static void IntoDict(const BackgroundTracingConfig* config, and, also puzzled why its static. seems like we should have virtual void scoped_ptr<DValue> AsDict() = 0; then the base has a private member [nonstatic] called IntoDict.
Message was sent while issue was closed.
On 2015/05/21 20:04:51, nduca wrote: > https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... > File content/public/browser/background_tracing_config.h (right): > > https://codereview.chromium.org/1089253003/diff/450001/content/public/browser... > content/public/browser/background_tracing_config.h:34: static void > IntoDict(const BackgroundTracingConfig* config, > and, also puzzled why its static. > > seems like we should have > > virtual void scoped_ptr<DValue> AsDict() = 0; > > > then the base has a private member [nonstatic] called IntoDict. Chatted with nduca offline, but summarizing here. Changes to the interface were to conform to the rules for content/public, ie. "content/public should contain only interfaces, structs and (rarely) static functions". Made the scoped_ptr<> change. About the tsan warning, looks like that was an existing issue that the browsertest happened to trigger. Filed a bug here: https://code.google.com/p/chromium/issues/detail?id=490856
The CQ bit was checked by simonhatch@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from nduca@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1089253003/#ps470001 (title: "Scoped_ptr")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/470001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by simonhatch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1089253003/470001
Message was sent while issue was closed.
Committed patchset #16 (id:470001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/0b016196499e8b5da3f20d90b201d90339ec88c8 Cr-Commit-Position: refs/heads/master@{#331141} |
