Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
171143002:
Implements Windows system tracing. (Closed)
|
Created:
6 years, 10 months ago by etienneb Modified:
6 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, dsinclair+watch_chromium.org, erikwright+watch_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@tracing Visibility:
Public. |
DescriptionImplements Windows system tracing.
This CL brings the tracing information into the resulting JSon.
As tracing needs administrator rights, we assume that Chrome is launched
as administrator.
To be able to view events, the traceviewer part must land first.
see: https://codereview.appspot.com/51300043/
BUG=
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255818
Patch Set 1 #Patch Set 2 : Remove leaky ETWController #Patch Set 3 : nits #
Total comments: 8
Patch Set 4 : nits #
Total comments: 10
Patch Set 5 : pull out the consumer to it's own file #Patch Set 6 : split cl again #Patch Set 7 : nits #Patch Set 8 : nits #Patch Set 9 : rebase on ToT #
Total comments: 10
Patch Set 10 : singleton #Patch Set 11 : stop thread execution #Patch Set 12 : nits #
Total comments: 6
Patch Set 13 : fix nduca comments #Patch Set 14 : #Patch Set 15 : nit #Patch Set 16 : fix license #Patch Set 17 : detect 64-bit kernel #Patch Set 18 : add clock sync #Patch Set 19 : remove detection of 64-bit kernel #Patch Set 20 : fix android build #
Messages
Total messages: 41 (0 generated)
looks solid, lets pull the consumer to its own file plus a few bits of feedback inline https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:55: thread_.Start(); you should probably kill the thread when things stop, too. maybe instead of having thread_ being a by-value member, make it be a scoped_ptr() that you create and destroy only for the lifetime of the trace https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:110: bool StartNTKernelSessionTracing() { can this be a private member and you just make there be only one public start function that first checks that kernel tracing can be started, and only then start the thread? https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:175: base::LazyInstance<EtwSystemEventConsumer>::Leaky g_etw_consumer = how about having tracing controller own this instance? tracing controller is itself a singelton... https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:858: #if defined(OS_WIN) lets pull out this and the cros one to if (is_system_tracing_) { BeginToStopSystemTracing(); return; } Then there's only one if for system tracing. The ifdefs can be inside the function body.
https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:101: value->Set("payload", base::Value::CreateStringValue(payload)); oh also should base64 encode?
https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:72: void AppendEventToBuffer(EVENT_TRACE* event) { Should be private. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:100: StringToLowerASCII(base::HexEncode(event->MofData, event->MofLength)); Too much indentation. Should be 4 spaces. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:141: No empty line.
Question, I don't know anything about ETW, but, would it be possible, instead of outputting an ETW event with a whole blob of data, could we output actual begin, end, instant, etc, events for each ETW event? That way they'd show up in the tracing timeline without any changes. Or, are there a lot more ETW types (in which case, could we convert the ones that make sense and output the remaining in the ETW blob?) https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:120: p.BufferSize = 16; // 16 K buffers. Should these be moved to constants instead of having comments?
Yes, this is possible but it's not what you want. There is many encoding version for each type of event. By decoding on the JS part (viewer) it make it possible to upgrade the Viewer an support a new type of event. On 2014/02/19 16:29:38, dsinclair wrote: > Question, I don't know anything about ETW, but, would it be possible, instead of > outputting an ETW event with a whole blob of data, could we output actual begin, > end, instant, etc, events for each ETW event? That way they'd show up in the > tracing timeline without any changes. > > Or, are there a lot more ETW types (in which case, could we convert the ones > that make sense and output the remaining in the ETW blob?) > > https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... > File content/browser/tracing/tracing_controller_impl.cc (right): > > https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... > content/browser/tracing/tracing_controller_impl.cc:120: p.BufferSize = 16; // > 16 K buffers. > Should these be moved to constants instead of having comments?
PTAnL. I addressed most of the comments. The main issue still present is when to kill the consumer thread. https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:55: thread_.Start(); I cannot kill the thread until the events isn't flushed. Any thoughs on that? On 2014/02/18 22:17:13, nduca wrote: > you should probably kill the thread when things stop, too. > > maybe instead of having thread_ being a by-value member, make it be a > scoped_ptr() that you create and destroy only for the lifetime of the trace https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:110: bool StartNTKernelSessionTracing() { I kept them separate to be able to update: is_system_tracing_. On 2014/02/18 22:17:13, nduca wrote: > can this be a private member and you just make there be only one public start > function that first checks that kernel tracing can be started, and only then > start the thread? https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:175: base::LazyInstance<EtwSystemEventConsumer>::Leaky g_etw_consumer = The ProcessEvent is called by a windows callback. Thus, we need to use the TracingControllerSingleton to get the TracingConsumer singleton. This way, we do not need to expose the consumer through the controller. I saw that as the Controller "use" the ETWController/ETWConsumer. On 2014/02/18 22:17:13, nduca wrote: > how about having tracing controller own this instance? tracing controller is > itself a singelton... https://codereview.chromium.org/171143002/diff/50001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:858: #if defined(OS_WIN) On 2014/02/18 22:17:13, nduca wrote: > lets pull out this and the cros one to > > if (is_system_tracing_) { > BeginToStopSystemTracing(); > return; > } > > Then there's only one if for system tracing. The ifdefs can be inside the > function body. Done. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:72: void AppendEventToBuffer(EVENT_TRACE* event) { On 2014/02/19 16:18:08, fdoray wrote: > Should be private. Done. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:100: StringToLowerASCII(base::HexEncode(event->MofData, event->MofLength)); On 2014/02/19 16:18:08, fdoray wrote: > Too much indentation. Should be 4 spaces. Done. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:101: value->Set("payload", base::Value::CreateStringValue(payload)); On 2014/02/18 22:17:46, nduca wrote: > oh also should base64 encode? Done. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:120: p.BufferSize = 16; // 16 K buffers. On 2014/02/19 16:29:39, dsinclair wrote: > Should these be moved to constants instead of having comments? Done. https://codereview.chromium.org/171143002/diff/80001/content/browser/tracing/... content/browser/tracing/tracing_controller_impl.cc:141: On 2014/02/19 16:18:08, fdoray wrote: > No empty line. Done.
getting close :) https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... File content/browser/tracing/tracing_consumer_win.cc (right): https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.cc:37: bool EtwSystemEventConsumer::StartKernelSessionTracing() { lets match the class name of this to the file, so etw_system_event_consumer.cc/h or etw_tracing_consumer_win.h and EtwTracingConsumer as the class name. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... File content/browser/tracing/tracing_consumer_win.h (right): https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:24: EtwSystemEventConsumer(); if its a singleton the constructor should be private and only Get should do it. using the singleton pattern makes this work. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:25: you need a private virtual destructor, even if its got an empty implemetnation. private so you cant delete it except from the implementation https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:35: void RequestStartTraceConsuming(); my request to keep this internal to the file remains. it may make sense to you but its hard to understand as a user of the code. you should have basically bool Start() --- if it doesn't start, then the systracE_state accordingly BeginToStop(callback&) --- stops the tracing, waits for it to drain, kills the thread, stops the callback Thats all there should be. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:42: // Retrieve the ETW consumer instance. forgive me if i asked this once but can't we use the standard singleton pattern? see base/debug/trace_event_impl.h's TraceLog for instance?
I added a callback to kill the thread and refactor the singleton pattern. PTAaanL. I hope it's close enough now. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... File content/browser/tracing/tracing_consumer_win.cc (right): https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.cc:37: bool EtwSystemEventConsumer::StartKernelSessionTracing() { On 2014/02/20 07:04:54, nduca wrote: > lets match the class name of this to the file, so etw_system_event_consumer.cc/h > or etw_tracing_consumer_win.h and EtwTracingConsumer as the class name. Done. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... File content/browser/tracing/tracing_consumer_win.h (right): https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:24: EtwSystemEventConsumer(); On 2014/02/20 07:04:54, nduca wrote: > if its a singleton the constructor should be private and only Get should do it. > using the singleton pattern makes this work. Done. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:25: On 2014/02/20 07:04:54, nduca wrote: > you need a private virtual destructor, even if its got an empty implemetnation. > private so you cant delete it except from the implementation Done. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:35: void RequestStartTraceConsuming(); On 2014/02/20 07:04:54, nduca wrote: > my request to keep this internal to the file remains. it may make sense to you > but its hard to understand as a user of the code. you should have basically > > bool Start() --- if it doesn't start, then the systracE_state accordingly > > BeginToStop(callback&) --- stops the tracing, waits for it to drain, kills > the thread, stops the callback > > Thats all there should be. Done. https://codereview.chromium.org/171143002/diff/230001/content/browser/tracing... content/browser/tracing/tracing_consumer_win.h:42: // Retrieve the ETW consumer instance. On 2014/02/20 07:04:54, nduca wrote: > forgive me if i asked this once but can't we use the standard singleton pattern? > see base/debug/trace_event_impl.h's TraceLog for instance? Done.
lgtm! https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... File content/browser/tracing/etw_system_event_consumer_win.h (right): https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... content/browser/tracing/etw_system_event_consumer_win.h:32: // Static override of EtwTraceConsumerBase::ProcessEvent. use friends so this is private? https://codereview.chromium.org/171143002/diff/400001/content/public/browser/... File content/public/browser/tracing_controller.h (left): https://codereview.chromium.org/171143002/diff/400001/content/public/browser/... content/public/browser/tracing_controller.h:153: // |callback| will will be called every time the given event occurs on any by changing this file you are requiring an additional OWNER reviewer. I Would suggest undoing the edit since its relatively minor [if anything at all?]
minor comment, lgtm still stands https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... File content/browser/tracing/etw_system_event_consumer_win.cc (right): https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... content/browser/tracing/etw_system_event_consumer_win.cc:57: if (!StopKernelSessionTracing()) little worried about this... if the kernel session tracing couldn't be stopped, should this be a critical error? The thread is now left running.... i think there's a way to do a criticail error that just takes down the browser process in this case?
done. This CL still needs an owner approval for content/content_browser.gypi (+darin). And must be landed after a roll-deps of chrome-tracing. https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... File content/browser/tracing/etw_system_event_consumer_win.cc (right): https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... content/browser/tracing/etw_system_event_consumer_win.cc:57: if (!StopKernelSessionTracing()) I agree. It can only happen if someone is playing with the controller outside of our scope. On 2014/02/21 05:23:43, nduca wrote: > little worried about this... if the kernel session tracing couldn't be stopped, > should this be a critical error? The thread is now left running.... i think > there's a way to do a criticail error that just takes down the browser process > in this case? https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... File content/browser/tracing/etw_system_event_consumer_win.h (right): https://codereview.chromium.org/171143002/diff/400001/content/browser/tracing... content/browser/tracing/etw_system_event_consumer_win.h:32: // Static override of EtwTraceConsumerBase::ProcessEvent. On 2014/02/21 05:22:10, nduca wrote: > use friends so this is private? Done. https://codereview.chromium.org/171143002/diff/400001/content/public/browser/... File content/public/browser/tracing_controller.h (left): https://codereview.chromium.org/171143002/diff/400001/content/public/browser/... content/public/browser/tracing_controller.h:153: // |callback| will will be called every time the given event occurs on any On 2014/02/21 05:22:10, nduca wrote: > by changing this file you are requiring an additional OWNER reviewer. I Would > suggest undoing the edit since its relatively minor [if anything at all?] Done.
You can TBR=darin for the gypi change so feel free to land.
GYPI change LGTM
I added a few changes to this CL. - remove the pretty-print of json -> used only for debugging - added the selection of QPC timer - added the 64-bit detection -> This was supposed to be detected by an EventTrace::Header event, but unfortunately, these events aren't sent with a RealTime tracing. see: http://msdn.microsoft.com/en-us/library/windows/desktop/aa363747(v=vs.85).aspx The event is supposed to be decoded and available in the TRACE_LOGFILE_HEADER structure. I added the code to grab this information and sent it into the JSON event. see: http://msdn.microsoft.com/en-us/library/windows/desktop/aa364145(v=vs.85).aspx
I added the clock sync events needed to synchronize kernel trace and chrome trace.
still lgtm :)
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/640001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on mac_rel for step(s) telemetry_perf_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
The CQ bit was checked by etienneb@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/etienneb@chromium.org/171143002/650001
Message was sent while issue was closed.
Change committed as 255818 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
