Implement system tracing for cros

content/browser/tracing/tracing_controller_impl.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/tracing/tracing_controller_impl.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/file_util.h"
 #include "base/json/string_escape.h"
 #include "base/strings/string_number_conversions.h"
 #include "content/browser/tracing/trace_message_filter.h"
 #include "content/browser/tracing/tracing_ui.h"
 #include "content/common/child_process_messages.h"
 #include "content/public/browser/browser_message_filter.h"
 #include "content/public/common/content_switches.h"
 
+#if defined(OS_CHROMEOS)
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/debug_daemon_client.h"
+#endif
+
 using base::debug::TraceLog;
 
 namespace content {
 
 namespace {
 
 base::LazyInstance<TracingControllerImpl>::Leaky g_controller =
     LAZY_INSTANCE_INITIALIZER;
 
 }  // namespace
@@ skipping 85 matching lines @@
 
 
 TracingControllerImpl::TracingControllerImpl() :
     pending_disable_recording_ack_count_(0),
     pending_capture_monitoring_snapshot_ack_count_(0),
     pending_trace_buffer_percent_full_ack_count_(0),
     maximum_trace_buffer_percent_full_(0),
     // Tracing may have been enabled by ContentMainRunner if kTraceStartup
     // is specified in command line.
     is_recording_(TraceLog::GetInstance()->IsEnabled()),
-    is_monitoring_(false) {
+    is_monitoring_(false),
+    is_system_tracing_(false) {
 }
 
 TracingControllerImpl::~TracingControllerImpl() {
   // This is a Leaky instance.
   NOTREACHED();
 }
 
 TracingControllerImpl* TracingControllerImpl::GetInstance() {
   return g_controller.Pointer();
 }
@@ skipping 53 matching lines @@
   if (pending_get_categories_done_callback_.is_null())
     TraceLog::GetInstance()->AddClockSyncMetadataEvent();
 #endif
 
   options_ = options;
   int trace_options = (options & RECORD_CONTINUOUSLY) ?
       TraceLog::RECORD_CONTINUOUSLY : TraceLog::RECORD_UNTIL_FULL;
   if (options & ENABLE_SAMPLING) {
     trace_options |=  TraceLog::ENABLE_SAMPLING;
   }
-  // TODO(haraken): How to handle ENABLE_SYSTRACE?
+  if (options & ENABLE_SYSTRACE) {

[dsinclair, 2014/01/30 14:36:06]

Is SYSTRACE only for cros, or will this if () also be used for Android? If just
CrOS, can we put the #define outside the if () so the branch gets pre-processed
out?

[haraken, 2014/01/31 05:58:27]

Done.

+#if defined(OS_CHROMEOS)
+    DCHECK(!is_system_tracing_);
+    chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->
+      StartSystemTracing();

[dsinclair, 2014/01/30 14:36:06]

nit: indenting.

[haraken, 2014/01/31 05:58:27]

I'm not sure but this is in the middle of an expression (not parameters of a
function), so it should be 2-space indented. The old code also used 2-space
indentation.

+    is_system_tracing_ = true;
+#endif
+  }
 
   base::Closure on_enable_recording_done_callback =
       base::Bind(&TracingControllerImpl::OnEnableRecordingDone,
                  base::Unretained(this),
                  category_filter,trace_options, callback);
   BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE,
       base::Bind(&TracingControllerImpl::SetEnabledOnFileThread,
                  base::Unretained(this),
                  category_filter,
                  base::debug::TraceLog::RECORDING_MODE,
@@ skipping 412 matching lines @@
   } else if (result_file_) {
     result_file_->Close(
         base::Bind(&TracingControllerImpl::OnResultFileClosed,
                    base::Unretained(this)));
   }
 }
 
 void TracingControllerImpl::OnResultFileClosed() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
+  if (is_system_tracing_) {
+    // Disable system tracing now that the local trace has shutdown.
+    // This must be done last because we potentially need to push event
+    // records into the system event log for synchronizing system event
+    // timestamps with chrome event timestamps--and since the system event
+    // log is a ring-buffer (on linux) adding them at the end is the only
+    // way we're confident we'll have them in the final result.
+    is_system_tracing_ = false;

[dsinclair, 2014/01/30 14:36:06]

This can only be set true inside a #if defined(OS_CHROMEOS) so can we move this
whole block inside the #if define below (including the outer if
(is_system_tracing_)?

[haraken, 2014/01/31 05:58:27]

Done.

+#if defined(OS_CHROMEOS)
+    chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->
+      RequestStopSystemTracing(

[dsinclair, 2014/01/30 14:36:06]

nit: indenting

+          base::Bind(&TracingMessageHandler::OnEndSystemTracingAcked,
+                     base::Unretained(this)));
+#endif
+    return;
+  }
+
   if (!result_file_)
     return;
 
   if (!pending_disable_recording_done_callback_.is_null()) {
     pending_disable_recording_done_callback_.Run(result_file_->path());
     pending_disable_recording_done_callback_.Reset();
   }
   result_file_.reset();
 }
 
+void TracingControllerImpl::OnEndSystemTracingAcked(
+    const scoped_refptr<base::RefCountedString>& events_str_ptr) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  TracingUI* tracing_ui = 0;

[haraken, 2014/01/30 13:39:46]

This is a problem. Since TracingController does not know which TracingUI called
TracingController::DisableRecording, TracingController cannot dispatch
OnSystemTraceDataCollected to a proper TracingUI.

Since TracingController::DisableRecording is called by static methods of
TracingUI, it is not trivial to teach TracingController which TracingUI called
TracingController::DisableRecording.

(Note: TracingController knows a list of existing TracingUIs. They are stored in
TracingControllerImpl::tracing_uis_.)

How can we address this issue?

[dsinclair, 2014/01/30 14:36:06]

So, does tracing_ui matter in this case? I though we wanted to dump the system
trace data into the file the same as we're dumping in regular trace data? I
guess the problem we have is that the JSON objects in the file are already
complete and we have no way to inject the extra systrace data into them correct?

[haraken, 2014/01/30 15:25:02]

I'm not familiar with how the system tracing was working, so I want to
understand :)

If we can just append system tracing data to the same JSON file, I think I can
implement that way.

I thought we need to call "tracingController.onSystemTraceDataCollected" since
the previous tracing_ui.cc was doing that (Line 553 of
https://codereview.chromium.org/23556003/diff/6001/content/browser/tracing/tr...).

[haraken, 2014/01/31 05:58:27]

Done. I changed the code so that the system tracing data is appended to
|result_file_|.

+  tracing_ui->OnSystemTraceDataCollected(events_str_ptr);
+  DCHECK(!is_system_tracing_);
+  OnResultFileClosed();
+}
+
 void TracingControllerImpl::OnCaptureMonitoringSnapshotAcked(
     TraceMessageFilter* trace_message_filter) {
   if (!BrowserThread::CurrentlyOn(BrowserThread::UI)) {
     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
         base::Bind(&TracingControllerImpl::OnCaptureMonitoringSnapshotAcked,
                    base::Unretained(this),
                    make_scoped_refptr(trace_message_filter)));
     return;
   }
 
@@ skipping 161 matching lines @@
   is_monitoring_ = is_monitoring;
 #if !defined(OS_ANDROID)
   for (std::set<TracingUI*>::iterator it = tracing_uis_.begin();
        it != tracing_uis_.end(); it++) {
     (*it)->OnMonitoringStateChanged(is_monitoring);
   }
 #endif
 }
 
 }  // namespace content