From db6916ca80a235f49970f382f0725622b894fc0d Mon Sep 17 00:00:00 2001 From: Krithika Balan Date: Mon, 22 Jul 2024 18:37:24 +0000 Subject: [PATCH] [trace controller] Update trace controller FIDL Split the trace controller protocol into Provisioner and Session protocols. The existing controller will be removed once all clients are migrated to the new protocol. Bug: b/42083286 Change-Id: I6cfa09afe58bac00205b956f14f067531df92bab Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1080794 Reviewed-by: Gwen Mittertreiner API-Review: Christopher Johnson Commit-Queue: Krithika Balan Reviewed-by: Christopher Johnson --- .../trace_controller.fidl | 86 ++- src/performance/trace_manager/app.cc | 23 +- src/performance/trace_manager/app.h | 25 +- .../trace_manager/tests/init_to_fini_tests.cc | 3 +- .../trace_manager/tests/trace_manager_test.cc | 9 + .../trace_manager/trace_manager.cc | 597 ++++++++++++------ src/performance/trace_manager/trace_manager.h | 100 ++- .../trace_manager/trace_session.cc | 61 +- src/performance/trace_manager/trace_session.h | 12 +- src/performance/trace_manager/tracee.cc | 2 +- 10 files changed, 672 insertions(+), 246 deletions(-) diff --git a/sdk/fidl/fuchsia.tracing.controller/trace_controller.fidl b/sdk/fidl/fuchsia.tracing.controller/trace_controller.fidl index 676d50d362fc..787b47a366c9 100644 --- a/sdk/fidl/fuchsia.tracing.controller/trace_controller.fidl +++ b/sdk/fidl/fuchsia.tracing.controller/trace_controller.fidl @@ -38,7 +38,41 @@ type SessionState = flexible enum { TERMINATING = 7; }; -/// The controller interface used by the trace tool to initialize/start/stop/terminate tracing. +/// The provisioner interface used by the trace tool to initialize tracing +/// +/// The provisioner is required to start a tracing session and bind the control +/// to the client. It can also perform tasks like getting the list of registered +/// providers and known categories, which can be performed without a existing +/// trace session. +@discoverable +open protocol Provisioner { + /// Requests to initialize tracing with the specified `config`. + /// + /// A bad request will terminate the connection. + /// + /// The trace controller emits trace data to `output` as a sequence of + /// binary formatted trace records. Traces obtained from different providers + /// are delimited by metadata records within the stream. + /// The format of these records can be found here: + /// https://fuchsia.dev/fuchsia-src/reference/tracing/trace-format + flexible InitializeTracing(resource struct { + controller server_end:Session; + config TraceConfig; + output zx.Handle:SOCKET; + }); + + /// Return the set of registered providers. + flexible GetProviders() -> (struct { + providers vector:MAX_NUM_PROVIDERS; + }); + + // Gets the known categories. + flexible GetKnownCategories() -> (struct { + categories vector:fuchsia.tracing.MAX_NUM_KNOWN_CATEGORIES; + }); +}; + +/// The controller interface used by the trace tool to start/stop/terminate a tracing session. /// /// The trace controller may lightly validate the structure of /// trace records as it copies them from trace buffers into the output. @@ -49,6 +83,46 @@ type SessionState = flexible enum { /// themselves. For example, it does not try to check argument lengths in /// events. This ensures that the trace format can be extended without needing /// to modify the trace controller. +open protocol Session { + /// Requests to start tracing with the specified `options`. + /// + /// If tracing has already started then the request is ignored, + /// except to send back an error code. + /// + /// The trace controller acknowledges the request when all + /// registered providers have been started or after + /// `TraceConfig.start_timeout_milliseconds` milliseconds. + /// One useful reason for the has-started acknowledgement is that the + /// trace program can start a program to trace knowing that all the + /// providers are started. + flexible StartTracing(struct { + options StartOptions; + }) -> () error StartErrorCode; + + /// Requests to stop tracing. + /// + /// If tracing has already stopped then this does nothing. + /// Returning a result lets callers know when it's ok to, for example, + /// start tracing again. + flexible StopTracing(struct { + options StopOptions; + }) -> (struct { + result TerminateResult; + }) error StopErrorCode; + + /// Returns the next alert when it arrives. + flexible WatchAlert() -> (struct { + alert_name AlertName; + }); + + /// Event sent when the session state changes. + flexible -> OnSessionStateChange(struct { + state SessionState; + }); +}; + +// Copy of the existing controller to keep things working until all references +// are to it are removed @discoverable protocol Controller { /// Requests to initialize tracing with the specified `config`. @@ -185,6 +259,16 @@ type StartErrorCode = flexible enum { TERMINATING = 4; }; +/// Error codes from Stop operations. +type StopErrorCode = flexible enum { + /// Trace controller is unavailable. Nothing to stop. + NOT_INITIALIZED = 1; + /// Tracing has not started or is currently stoppped. + NOT_STARTED = 2; + /// Tracing aborted due to error. + ABORTED = 3; +}; + /// Additional options to control trace data collection. type StartOptions = table { /// Whether and how to clear the buffer when starting data collection. diff --git a/src/performance/trace_manager/app.cc b/src/performance/trace_manager/app.cc index 74c0649217bd..8c0607ae9461 100644 --- a/src/performance/trace_manager/app.cc +++ b/src/performance/trace_manager/app.cc @@ -12,20 +12,37 @@ namespace tracing { TraceManagerApp::TraceManagerApp(std::unique_ptr context, Config config, async::Executor& executor) - : context_(std::move(context)), trace_manager_(this, std::move(config), executor) { + : context_(std::move(context)), + trace_manager_(this, std::move(config), executor), + old_trace_manager_(this, &trace_manager_, executor) { [[maybe_unused]] zx_status_t status; status = context_->outgoing()->AddPublicService( provider_registry_bindings_.GetHandler(&trace_manager_)); FX_DCHECK(status == ZX_OK); - status = context_->outgoing()->AddPublicService(controller_bindings_.GetHandler(&trace_manager_)); + status = context_->outgoing()->AddPublicService( + old_controller_bindings_.GetHandler(&old_trace_manager_)); + FX_DCHECK(status == ZX_OK); + old_controller_bindings_.set_empty_set_handler( + [this]() { old_trace_manager_.OnEmptyControllerSet(); }); + + status = + context_->outgoing()->AddPublicService(provisioner_bindings_.GetHandler(&trace_manager_)); FX_DCHECK(status == ZX_OK); - controller_bindings_.set_empty_set_handler([this]() { trace_manager_.OnEmptyControllerSet(); }); FX_LOGS(DEBUG) << "TraceManager services registered"; } +void TraceManagerApp::AddSessionBinding( + std::shared_ptr trace_session, + fidl::InterfaceRequest session_controller) { + session_bindings_.AddBinding(trace_session, std::move(session_controller)); + session_bindings_.set_empty_set_handler([this]() { trace_manager_.OnEmptyControllerSet(); }); + + FX_LOGS(DEBUG) << "TraceController registered"; +} + TraceManagerApp::~TraceManagerApp() = default; } // namespace tracing diff --git a/src/performance/trace_manager/app.h b/src/performance/trace_manager/app.h index 853472836c09..e52d04d362a2 100644 --- a/src/performance/trace_manager/app.h +++ b/src/performance/trace_manager/app.h @@ -22,12 +22,23 @@ class TraceManagerApp { async::Executor& executor); ~TraceManagerApp(); + void AddSessionBinding(std::shared_ptr trace_session, + fidl::InterfaceRequest session_controller); + + void CloseSessionBindings() { session_bindings_.CloseAll(); } + // For testing. sys::ComponentContext* context() const { return context_.get(); } const TraceManager* trace_manager() const { return &trace_manager_; } + const OldTraceManager* old_trace_manager() const { return &old_trace_manager_; } const fidl::BindingSet& controller_bindings() const { - return controller_bindings_; + return old_controller_bindings_; + } + + const fidl::BindingSet>& + session_bindings() const { + return session_bindings_; } private: @@ -35,8 +46,18 @@ class TraceManagerApp { TraceManager trace_manager_; + // TODO(b/42083286): Remove after the old trace Controller protocol is removed + // This is present to maintain backwards comparitibility during the process of + // migrating to use the new trace protocols. This will be removed once all tracing + // clients are migrated. + OldTraceManager old_trace_manager_; + fidl::BindingSet provider_registry_bindings_; - fidl::BindingSet controller_bindings_; + fidl::BindingSet old_controller_bindings_; + fidl::BindingSet provisioner_bindings_; + fidl::BindingSet> + session_bindings_; TraceManagerApp(const TraceManagerApp&) = delete; TraceManagerApp(TraceManagerApp&&) = delete; diff --git a/src/performance/trace_manager/tests/init_to_fini_tests.cc b/src/performance/trace_manager/tests/init_to_fini_tests.cc index eb46c73044c8..2ca942187499 100644 --- a/src/performance/trace_manager/tests/init_to_fini_tests.cc +++ b/src/performance/trace_manager/tests/init_to_fini_tests.cc @@ -131,7 +131,8 @@ TEST_F(TraceManagerTest, InitToFiniWithNoStop) { VerifyCounts(1, 0); ASSERT_TRUE(TerminateSession()); - VerifyCounts(1, 0); + // Stop is immplicitly called first before terminating. + VerifyCounts(1, 1); } static constexpr char kAlertName[] = "alert-name"; diff --git a/src/performance/trace_manager/tests/trace_manager_test.cc b/src/performance/trace_manager/tests/trace_manager_test.cc index cfb93beb9321..aaf9b5ada1cc 100644 --- a/src/performance/trace_manager/tests/trace_manager_test.cc +++ b/src/performance/trace_manager/tests/trace_manager_test.cc @@ -317,6 +317,15 @@ bool TraceManagerTest::FinishTerminateSession(controller::TerminateResult* resul if (fake_provider_bindings().size() > 0) { FX_LOGS(DEBUG) << "Loop done, expecting session terminating"; SessionState state = GetSessionState(); + if (state == SessionState::kStopping) { + // Terminate issued without stop will implicitly call stop first + // Loop will exit for the transition to kStopping. Mark providers + // as stopped and run the loop twice to transition to kStopped, + // then kTerminating + MarkAllProvidersStopped(); + RunLoopUntilIdle(); + } + state = GetSessionState(); EXPECT_EQ(state, SessionState::kTerminating); if (state != SessionState::kTerminating) { return false; diff --git a/src/performance/trace_manager/trace_manager.cc b/src/performance/trace_manager/trace_manager.cc index 1feb91ba6530..7fbd773e23fd 100644 --- a/src/performance/trace_manager/trace_manager.cc +++ b/src/performance/trace_manager/trace_manager.cc @@ -56,32 +56,209 @@ using KnownCategoryVector = std::vector; } // namespace +TraceController::TraceController(TraceManagerApp* app, std::unique_ptr session) + : app_(app), session_(std::move(session)) { + session_->MarkInitialized(); +} + +TraceController::~TraceController() = default; + +// fidl +void TraceController::handle_unknown_method(uint64_t ordinal, bool method_has_response) { + FX_LOGS(WARNING) << "Received an unknown method with ordinal " << ordinal; +} + +// fidl +void TraceController::StartTracing(controller::StartOptions options, + StartTracingCallback start_callback) { + FX_LOGS(DEBUG) << "StartTracing"; + + controller::Session_StartTracing_Result result; + + if (!session_) { + FX_LOGS(ERROR) << "Ignoring start request, trace must be initialized first"; + result.set_err(controller::StartErrorCode::NOT_INITIALIZED); + start_callback(std::move(result)); + return; + } + + switch (session_->state()) { + case TraceSession::State::kStarting: + case TraceSession::State::kStarted: + FX_LOGS(ERROR) << "Ignoring start request, trace already started"; + result.set_err(controller::StartErrorCode::ALREADY_STARTED); + start_callback(std::move(result)); + return; + case TraceSession::State::kStopping: + FX_LOGS(ERROR) << "Ignoring start request, trace stopping"; + result.set_err(controller::StartErrorCode::STOPPING); + start_callback(std::move(result)); + return; + case TraceSession::State::kTerminating: + FX_LOGS(ERROR) << "Ignoring start request, trace terminating"; + result.set_err(controller::StartErrorCode::TERMINATING); + start_callback(std::move(result)); + return; + case TraceSession::State::kInitialized: + case TraceSession::State::kStopped: + break; + default: + FX_NOTREACHED(); + return; + } + + std::vector additional_categories; + if (options.has_additional_categories()) { + additional_categories = std::move(options.additional_categories()); + } + + // This default matches trace's. + fuchsia::tracing::BufferDisposition buffer_disposition = + fuchsia::tracing::BufferDisposition::RETAIN; + if (options.has_buffer_disposition()) { + buffer_disposition = options.buffer_disposition(); + switch (buffer_disposition) { + case fuchsia::tracing::BufferDisposition::CLEAR_ENTIRE: + case fuchsia::tracing::BufferDisposition::CLEAR_NONDURABLE: + case fuchsia::tracing::BufferDisposition::RETAIN: + break; + default: + FX_LOGS(ERROR) << "Bad value for buffer disposition: " << buffer_disposition + << ", dropping connection"; + // TODO(dje): IWBN to drop the connection. How? + result.set_err(controller::StartErrorCode::TERMINATING); + start_callback(std::move(result)); + return; + } + } + + FX_LOGS(INFO) << "Starting trace, buffer disposition: " << buffer_disposition; + + session_->Start(buffer_disposition, additional_categories, std::move(start_callback)); +} + +// fidl +void TraceController::StopTracing(controller::StopOptions options, + StopTracingCallback stop_callback) { + controller::Session_StopTracing_Result stop_result; + + if (session_->state() != TraceSession::State::kInitialized && + session_->state() != TraceSession::State::kStarting && + session_->state() != TraceSession::State::kStarted) { + FX_LOGS(INFO) << "Ignoring stop request, state != Initialized,Starting,Started"; + stop_result.set_err(controller::StopErrorCode::NOT_STARTED); + stop_callback(std::move(stop_result)); + return; + } + + bool write_results = false; + if (options.has_write_results()) { + write_results = options.write_results(); + } + + FX_LOGS(INFO) << "Stopping trace" << (write_results ? ", and writing results" : ""); + session_->Stop(write_results, [stop_callback = std::move(stop_callback)]( + controller::Session_StopTracing_Result result) { + FX_LOGS(DEBUG) << "Stopped trace"; + stop_callback(std::move(result)); + }); +} + +void TraceController::TerminateTracing(controller::TerminateOptions options, fit::closure cb) { + // Check the state first because the log messages are useful, but not if + // tracing has ended. + if (session_->state() == TraceSession::State::kTerminating) { + FX_LOGS(INFO) << "Ignoring terminate request. Already terminating"; + return; + } + + if (options.has_write_results()) { + session_->set_write_results_on_terminate(options.write_results()); + } + + session_->Terminate([this, callback = std::move(cb)]() { + FX_LOGS(DEBUG) << "Session Terminated"; + + // Clean up any bindings for currently running trace so a new one + // can be initiated + session_.reset(); + + FX_DCHECK(callback); + // The call back will destroy the TraceController object. Only issue the callback + // as the last thing to do. + callback(); + }); +} + TraceManager::TraceManager(TraceManagerApp* app, Config config, async::Executor& executor) : app_(app), config_(std::move(config)), executor_(executor) {} TraceManager::~TraceManager() = default; +void TraceManager::CloseSession() { + // Clean up any bindings for the currently running trace so a new one + // can be initiated. + + // The actual trace_controller object is held by app.SessionBindings + // and will be removed once the binding is closed. Remove the stored + // referencce to the trace_controller object to prevent use after free + FX_LOGS(DEBUG) << "Clean up leftover bindings"; + app_->CloseSessionBindings(); + trace_controller_.reset(); +} + void TraceManager::OnEmptyControllerSet() { // While one controller could go away and another remain causing a trace // to not be terminated, at least handle the common case. - FX_LOGS(DEBUG) << "Controller is gone"; - if (session_) { - // Check the state first because the log messages are useful, but not if - // tracing has ended. - if (session_->state() != TraceSession::State::kTerminating) { - FX_LOGS(INFO) << "Controller is gone, terminating trace"; - session_->Terminate([this](controller::Controller_TerminateTracing_Result result) { - FX_LOGS(INFO) << "Trace terminated"; - session_.reset(); - }); - } + FX_LOGS(INFO) << "Controller is gone"; + if (trace_controller_) { + FX_LOGS(DEBUG) << "Terminating trace and closing session"; + controller::TerminateOptions options; + // Terminate the running trace and the close the trace session. + trace_controller_->TerminateTracing(std::move(options), [this]() { CloseSession(); }); + } else { + CloseSession(); } +} - while (!watch_alert_callbacks_.empty()) { - watch_alert_callbacks_.pop(); - } - while (!alerts_.empty()) { - alerts_.pop(); +void TraceManager::TerminateTracing( + controller::TerminateOptions options, + fit::function cb) { + if (session() && session()->state() != TraceSession::State::kTerminating) { + FX_LOGS(DEBUG) << "Stop and Terminate tracing"; + // Trace may still be running. Make sure we stop first in order to get stats. + controller::StopOptions stop_options; + stop_options.set_write_results((options.has_write_results() && options.write_results())); + + fpromise::bridge bridge; + trace_controller_->StopTracing( + std::move(stop_options), [completer = std::move(bridge.completer)]( + controller::Session_StopTracing_Result stop_result) mutable { + if (stop_result.is_response()) { + completer.complete_ok(std::move(stop_result.response().result)); + } else { + std::vector stats; + controller::TerminateResult terminate_result; + terminate_result.set_provider_stats(std::move(stats)); + completer.complete_ok(std::move(terminate_result)); + } + }); + auto terminate_promise = bridge.consumer.promise().then( + [this, cb = std::move(cb)](fpromise::result& result) mutable { + controller::TerminateOptions options; + trace_controller_->TerminateTracing(std::move(options), [this, result = std::move(result), + callback = + std::move(cb)]() mutable { + controller::Controller_TerminateTracing_Result terminate_result; + controller::Controller_TerminateTracing_Response response(std::move(result.value())); + terminate_result.set_response(std::move(response)); + CloseSession(); + FX_DCHECK(callback); + callback(std::move(terminate_result)); + }); + }); + + executor_.schedule_task(std::move(terminate_promise)); } } @@ -91,10 +268,11 @@ void TraceManager::handle_unknown_method(uint64_t ordinal, bool method_has_respo } // fidl -void TraceManager::InitializeTracing(controller::TraceConfig config, zx::socket output) { +void TraceManager::InitializeTracing(fidl::InterfaceRequest controller, + controller::TraceConfig config, zx::socket output) { FX_LOGS(DEBUG) << "InitializeTracing"; - if (session_) { + if (trace_controller_) { FX_LOGS(ERROR) << "Ignoring initialize request, trace already initialized"; return; } @@ -160,171 +338,35 @@ void TraceManager::InitializeTracing(controller::TraceConfig config, zx::socket start_timeout = zx::msec(config.start_timeout_milliseconds()); } - session_ = std::make_unique( + auto session = std::make_unique( executor_, std::move(output), std::move(categories), default_buffer_size_megabytes, tracing_buffering_mode, std::move(provider_specs), start_timeout, kStopTimeout, [this]() { - if (session_->state() != TraceSession::State::kTerminating) { - FX_LOGS(INFO) << "Aborting and terminating trace"; - session_->Terminate([this](controller::Controller_TerminateTracing_Result result) { - FX_LOGS(INFO) << "Terminated trace"; - session_.reset(); - }); + if (trace_controller_) { + controller::TerminateOptions options; + options.set_write_results(false); + trace_controller_->TerminateTracing(std::move(options), [this]() { CloseSession(); }); } }, - [this](const std::string& alert_name) { OnAlert(alert_name); }); + [this](const std::string& alert_name) { trace_controller_->OnAlert(alert_name); }); // The trace header is written now to ensure it appears first, and to avoid // timing issues if the trace is terminated early (and the session being // deleted). - session_->WriteTraceInfo(); + session->WriteTraceInfo(); for (auto& bundle : providers_) { - session_->AddProvider(&bundle); - } - - session_->MarkInitialized(); -} - -// fidl -void TraceManager::TerminateTracing(controller::TerminateOptions options, - TerminateTracingCallback terminate_callback) { - controller::Controller_TerminateTracing_Result result; - controller::TerminateResult terminate_result; - if (!session_) { - FX_LOGS(DEBUG) << "Ignoring terminate request, tracing not initialized"; - result.set_response( - controller::Controller_TerminateTracing_Response(std::move(terminate_result))); - terminate_callback(std::move(result)); - return; - } - - if (session_->state() == TraceSession::State::kTerminating) { - FX_LOGS(INFO) << "Ignoring terminate request. Already terminating"; - result.set_response( - controller::Controller_TerminateTracing_Response(std::move(terminate_result))); - terminate_callback(std::move(result)); - return; - } - - if (options.has_write_results()) { - session_->set_write_results_on_terminate(options.write_results()); - } - - FX_LOGS(INFO) << "Terminating trace"; - session_->Terminate([this, terminate_callback = std::move(terminate_callback)]( - controller::Controller_TerminateTracing_Result result) { - terminate_callback(std::move(result)); - session_.reset(); - }); -} - -// fidl -void TraceManager::StartTracing(controller::StartOptions options, - StartTracingCallback start_callback) { - FX_LOGS(DEBUG) << "StartTracing"; - - controller::Controller_StartTracing_Result result; - - if (!session_) { - FX_LOGS(ERROR) << "Ignoring start request, trace must be initialized first"; - result.set_err(controller::StartErrorCode::NOT_INITIALIZED); - start_callback(std::move(result)); - return; - } - - switch (session_->state()) { - case TraceSession::State::kStarting: - case TraceSession::State::kStarted: - FX_LOGS(ERROR) << "Ignoring start request, trace already started"; - result.set_err(controller::StartErrorCode::ALREADY_STARTED); - start_callback(std::move(result)); - return; - case TraceSession::State::kStopping: - FX_LOGS(ERROR) << "Ignoring start request, trace stopping"; - result.set_err(controller::StartErrorCode::STOPPING); - start_callback(std::move(result)); - return; - case TraceSession::State::kTerminating: - FX_LOGS(ERROR) << "Ignoring start request, trace terminating"; - result.set_err(controller::StartErrorCode::TERMINATING); - start_callback(std::move(result)); - return; - case TraceSession::State::kInitialized: - case TraceSession::State::kStopped: - break; - default: - FX_NOTREACHED(); - return; - } - - std::vector additional_categories; - if (options.has_additional_categories()) { - additional_categories = std::move(options.additional_categories()); - } - - // This default matches trace's. - fuchsia::tracing::BufferDisposition buffer_disposition = - fuchsia::tracing::BufferDisposition::RETAIN; - if (options.has_buffer_disposition()) { - buffer_disposition = options.buffer_disposition(); - switch (buffer_disposition) { - case fuchsia::tracing::BufferDisposition::CLEAR_ENTIRE: - case fuchsia::tracing::BufferDisposition::CLEAR_NONDURABLE: - case fuchsia::tracing::BufferDisposition::RETAIN: - break; - default: - FX_LOGS(ERROR) << "Bad value for buffer disposition: " << buffer_disposition - << ", dropping connection"; - // TODO(dje): IWBN to drop the connection. How? - result.set_err(controller::StartErrorCode::TERMINATING); - start_callback(std::move(result)); - return; - } - } - - FX_LOGS(INFO) << "Starting trace, buffer disposition: " << buffer_disposition; - - session_->Start(buffer_disposition, additional_categories, std::move(start_callback)); -} - -// fidl -void TraceManager::StopTracing(controller::StopOptions options, StopTracingCallback stop_callback) { - controller::Controller_StopTracing_Result stop_result; - controller::Controller_StopTracing_Response response; - if (!session_) { - FX_LOGS(DEBUG) << "Ignoring stop request, tracing not started"; - stop_result.set_response(response); - stop_callback(std::move(stop_result)); - return; - } - - if (session_->state() != TraceSession::State::kInitialized && - session_->state() != TraceSession::State::kStarting && - session_->state() != TraceSession::State::kStarted) { - FX_LOGS(DEBUG) << "Ignoring stop request, state != Initialized,Starting,Started"; - stop_result.set_response(response); - stop_callback(std::move(stop_result)); - return; - } - - bool write_results = false; - if (options.has_write_results()) { - write_results = options.write_results(); + session->AddProvider(&bundle); } - FX_LOGS(INFO) << "Stopping trace" << (write_results ? ", and writing results" : ""); - session_->Stop(write_results, [stop_callback = std::move(stop_callback)]( - controller::Controller_StopTracing_Result result) { - FX_LOGS(INFO) << "Stopped trace"; - stop_callback(std::move(result)); - }); + trace_controller_ = std::make_shared(app_, std::move(session)); + app_->AddSessionBinding(trace_controller_, std::move(controller)); } // fidl void TraceManager::GetProviders(GetProvidersCallback callback) { FX_LOGS(DEBUG) << "GetProviders"; - controller::Controller_GetProviders_Result result; + controller::Provisioner_GetProviders_Result result; std::vector provider_info; for (const auto& provider : providers_) { controller::ProviderInfo info; @@ -333,7 +375,7 @@ void TraceManager::GetProviders(GetProvidersCallback callback) { info.set_name(provider.name); provider_info.push_back(std::move(info)); } - result.set_response(controller::Controller_GetProviders_Response(std::move(provider_info))); + result.set_response(controller::Provisioner_GetProviders_Response(std::move(provider_info))); callback(std::move(result)); } @@ -396,8 +438,8 @@ void TraceManager::GetKnownCategories(GetKnownCategoriesCallback callback) { result_known_categories.end()); } } - controller::Controller_GetKnownCategories_Result result; - result.set_response(controller::Controller_GetKnownCategories_Response( + controller::Provisioner_GetKnownCategories_Result result; + result.set_response(controller::Provisioner_GetKnownCategories_Response( {known_categories.begin(), known_categories.end()})); callback(std::move(result)); }); @@ -406,13 +448,13 @@ void TraceManager::GetKnownCategories(GetKnownCategoriesCallback callback) { executor_.schedule_task(std::move(timeout)); } -void TraceManager::WatchAlert(WatchAlertCallback cb) { +void TraceController::WatchAlert(WatchAlertCallback cb) { FX_LOGS(DEBUG) << "WatchAlert"; if (alerts_.empty()) { watch_alert_callbacks_.push(std::move(cb)); } else { - controller::Controller_WatchAlert_Result result; - result.set_response(controller::Controller_WatchAlert_Response(std::move(alerts_.front()))); + controller::Session_WatchAlert_Result result; + result.set_response(controller::Session_WatchAlert_Response(std::move(alerts_.front()))); cb(std::move(result)); alerts_.pop(); } @@ -423,15 +465,15 @@ void TraceManager::RegisterProviderWorker(fidl::InterfaceHandleprovider.set_error_handler([this, it](zx_status_t status) { - if (session_) - session_->RemoveDeadProvider(&(*it)); + if (session()) { + session()->RemoveDeadProvider(&(*it)); + } providers_.erase(it); }); - if (session_) { - session_->AddProvider(&(*it)); + if (session()) { + session()->AddProvider(&(*it)); } } @@ -446,18 +488,26 @@ void TraceManager::RegisterProviderSynchronously(fidl::InterfaceHandlestate() == TraceSession::State::kStarting || - session_->state() == TraceSession::State::kStarted)); + auto session_ptr = session(); + bool already_started = (session_ptr && (session_ptr->state() == TraceSession::State::kStarting || + session_ptr->state() == TraceSession::State::kStarted)); callback(ZX_OK, already_started); } -void TraceManager::SendSessionStateEvent(controller::SessionState state) { - for (const auto& binding : app_->controller_bindings().bindings()) { +void TraceController::SendSessionStateEvent(controller::SessionState state) { + for (const auto& binding : app_->session_bindings().bindings()) { binding->events().OnSessionStateChange(state); } } -controller::SessionState TraceManager::TranslateSessionState(TraceSession::State state) { +TraceSession* TraceManager::session() const { + if (trace_controller_) { + return trace_controller_->session(); + } + return nullptr; +} + +controller::SessionState TraceController::TranslateSessionState(TraceSession::State state) { switch (state) { case TraceSession::State::kReady: return controller::SessionState::READY; @@ -476,7 +526,7 @@ controller::SessionState TraceManager::TranslateSessionState(TraceSession::State } } -void TraceManager::OnAlert(const std::string& alert_name) { +void TraceController::OnAlert(const std::string& alert_name) { if (watch_alert_callbacks_.empty()) { if (alerts_.size() == kMaxAlertQueueDepth) { // We're at our queue depth limit. Discard the oldest alert. @@ -487,10 +537,189 @@ void TraceManager::OnAlert(const std::string& alert_name) { return; } - controller::Controller_WatchAlert_Result result; - result.set_response(controller::Controller_WatchAlert_Response(alert_name)); + controller::Session_WatchAlert_Result result; + result.set_response(controller::Session_WatchAlert_Response(alert_name)); watch_alert_callbacks_.front()(std::move(result)); watch_alert_callbacks_.pop(); } +OldTraceManager::OldTraceManager(TraceManagerApp* app, TraceManager* trace_manager, + async::Executor& executor) + : app_(app), trace_manager_(trace_manager), executor_(executor) { + FX_DCHECK(trace_manager_); +} + +OldTraceManager::~OldTraceManager() = default; + +void OldTraceManager::OnEmptyControllerSet() { + if (trace_controller_.is_bound()) { + trace_manager_->OnEmptyControllerSet(); + } + trace_controller_ = fidl::InterfacePtr(); +} + +// fidl +void OldTraceManager::InitializeTracing(controller::TraceConfig config, zx::socket output) { + if (trace_controller_.is_bound()) { + FX_LOGS(ERROR) << "Ignoring initialize request, trace already initialized"; + return; + } + + trace_manager_->InitializeTracing(trace_controller_.NewRequest(), std::move(config), + std::move(output)); +} + +// fidl +void OldTraceManager::TerminateTracing(controller::TerminateOptions options, + TerminateTracingCallback callback) { + if (!trace_controller_.is_bound()) { + FX_LOGS(ERROR) << "Ignoring terminate request, trace not initialized"; + controller::Controller_TerminateTracing_Result result; + controller::TerminateResult terminate_result; + result.set_response( + controller::Controller_TerminateTracing_Response(std::move(terminate_result))); + FX_DCHECK(callback); + callback(std::move(result)); + return; + } + + // The existing Controller allows tracing to be terminated without being stopped first and returns + // provider stats on terminate. The new protocol will return stats on terminate and will + // automatically clean up on dropping the fidl interface. Explicit termination is not required. + + // For now, trace_manager will do the proper clean-up. It will stop the trace if it is running, + // get the provider stats, terminate the providers, close the session and return the stats + // collected after stop. + trace_manager_->TerminateTracing( + std::move(options), [this, terminate_cb = std::move(callback)]( + controller::Controller_TerminateTracing_Result res) { + controller::Controller_TerminateTracing_Result result; + if (res.is_response() && !res.response().result.provider_stats().empty()) { + result = std::move(res); + } else { + result.set_response( + controller::Controller_TerminateTracing_Response(std::move(terminate_result_))); + } + FX_DCHECK(terminate_cb); + terminate_cb(std::move(result)); + trace_controller_ = fidl::InterfacePtr(); + }); +} + +// fidl +void OldTraceManager::StartTracing(controller::StartOptions options, + StartTracingCallback callback) { + if (!trace_controller_.is_bound()) { + FX_LOGS(ERROR) << "Ignoring start request. Trace not initialized"; + controller::Controller_StartTracing_Result result; + result.set_err(controller::StartErrorCode::NOT_INITIALIZED); + FX_DCHECK(callback); + callback(std::move(result)); + return; + } + + trace_controller_->StartTracing( + std::move(options), + [callback = std::move(callback)](controller::Session_StartTracing_Result start_result) { + controller::Controller_StartTracing_Result result; + if (start_result.is_err()) { + result.set_err(start_result.err()); + } else { + controller::Controller_StartTracing_Response response; + result.set_response(response); + } + FX_DCHECK(callback); + callback(std::move(result)); + }); +} + +// fidl +void OldTraceManager::StopTracing(controller::StopOptions options, StopTracingCallback callback) { + if (!trace_controller_.is_bound()) { + FX_LOGS(ERROR) << "Ignoring stop request. Trace not initialized"; + controller::Controller_StopTracing_Result result; + controller::Controller_StopTracing_Response response; + result.set_response(response); + FX_DCHECK(callback); + callback(std::move(result)); + return; + } + + trace_controller_->StopTracing( + std::move(options), + [this, callback = std::move(callback)](controller::Session_StopTracing_Result stop_result) { + if (stop_result.is_response()) { + terminate_result_ = std::move(stop_result.response().result); + } + controller::Controller_StopTracing_Result result; + controller::Controller_StopTracing_Response response; + result.set_response(response); + FX_DCHECK(callback); + callback(std::move(result)); + }); +} + +// fidl +void OldTraceManager::GetProviders(GetProvidersCallback callback) { + trace_manager_->GetProviders([callback = std::move(callback)]( + controller::Provisioner_GetProviders_Result providers_result) { + controller::Controller_GetProviders_Result result; + std::vector provider_info; + if (providers_result.is_response()) { + provider_info = std::move(providers_result.response().providers); + } + result.set_response(controller::Controller_GetProviders_Response(std::move(provider_info))); + FX_DCHECK(callback); + callback(std::move(result)); + }); +} + +// fidl +void OldTraceManager::GetKnownCategories(GetKnownCategoriesCallback callback) { + trace_manager_->GetKnownCategories( + [callback = std::move(callback)]( + controller::Provisioner_GetKnownCategories_Result categories_result) { + controller::Controller_GetKnownCategories_Result result; + KnownCategoryVector categories; + if (categories_result.is_response()) { + categories = std::move(categories_result.response().categories); + } + result.set_response( + controller::Controller_GetKnownCategories_Response(std::move(categories))); + FX_DCHECK(callback); + callback(std::move(result)); + }); +} + +// fidl +void OldTraceManager::WatchAlert(WatchAlertCallback callback) { + trace_controller_->WatchAlert( + [callback = std::move(callback)](controller::Session_WatchAlert_Result alert_result) { + controller::Controller_WatchAlert_Result result; + if (alert_result.is_response()) { + result.set_response(controller::Controller_WatchAlert_Response( + std::move(alert_result.response().alert_name))); + } + FX_DCHECK(callback); + callback(std::move(result)); + }); +} + +// fidl +void OldTraceManager::handle_unknown_method(uint64_t ordinal, bool method_has_response) { + FX_LOGS(WARNING) << "Received an unknown method with ordinal " << ordinal; +} + +void OldTraceManager::SendSessionStateEvent(controller::SessionState state) { + trace_manager_->trace_controller_->SendSessionStateEvent(state); +} + +controller::SessionState OldTraceManager::TranslateSessionState(TraceSession::State state) { + return trace_manager_->trace_controller_->TranslateSessionState(state); +} + +void OldTraceManager::OnAlert(const std::string& alert_name) { + trace_manager_->trace_controller_->OnAlert(alert_name); +} + } // namespace tracing diff --git a/src/performance/trace_manager/trace_manager.h b/src/performance/trace_manager/trace_manager.h index 159987b97dd6..cfe159ccdebe 100644 --- a/src/performance/trace_manager/trace_manager.h +++ b/src/performance/trace_manager/trace_manager.h @@ -29,26 +29,64 @@ namespace provider = fuchsia::tracing::provider; // forward decl, here to break mutual header dependency class TraceManagerApp; +class TraceManager; +class OldTraceManager; + +class TraceController : public controller::Session { + friend TraceManager; + friend OldTraceManager; + + public: + TraceController(TraceManagerApp* app, std::unique_ptr session); + ~TraceController() override; + + void TerminateTracing(controller::TerminateOptions options, fit::closure cb); + + void OnAlert(const std::string& alert_name); + + // For testing. + TraceSession* session() const { return session_.get(); } + + private: + // |Session| implementation. + void StartTracing(controller::StartOptions options, StartTracingCallback cb) override; + void StopTracing(controller::StopOptions options, StopTracingCallback cb) override; + void WatchAlert(WatchAlertCallback cb) override; + void handle_unknown_method(uint64_t ordinal, bool method_has_response) override; + + void SendSessionStateEvent(controller::SessionState state); + controller::SessionState TranslateSessionState(TraceSession::State state); + + TraceManagerApp* const app_; + + std::unique_ptr session_; + std::queue alerts_; + std::queue watch_alert_callbacks_; +}; + +class TraceManager : public controller::Provisioner, public provider::Registry { + friend TraceController; + friend OldTraceManager; -class TraceManager : public controller::Controller, public provider::Registry { public: TraceManager(TraceManagerApp* app, Config config, async::Executor& executor); ~TraceManager() override; // For testing. - const TraceSession* session() const { return session_.get(); } + TraceSession* session() const; + + // Allow terminate tracing with callback for testing + void TerminateTracing(controller::TerminateOptions options, + fit::function cb); void OnEmptyControllerSet(); private: - // |Controller| implementation. - void InitializeTracing(controller::TraceConfig config, zx::socket output) override; - void TerminateTracing(controller::TerminateOptions options, TerminateTracingCallback cb) override; - void StartTracing(controller::StartOptions options, StartTracingCallback cb) override; - void StopTracing(controller::StopOptions options, StopTracingCallback cb) override; + // |Provisioner| implementation. + void InitializeTracing(fidl::InterfaceRequest controller, + controller::TraceConfig config, zx::socket output) override; void GetProviders(GetProvidersCallback cb) override; void GetKnownCategories(GetKnownCategoriesCallback callback) override; - void WatchAlert(WatchAlertCallback cb) override; void handle_unknown_method(uint64_t ordinal, bool method_has_response) override; // |TraceRegistry| implementation. @@ -60,20 +98,15 @@ class TraceManager : public controller::Controller, public provider::Registry { uint64_t pid, std::string name, RegisterProviderSynchronouslyCallback callback) override; - void SendSessionStateEvent(controller::SessionState state); - controller::SessionState TranslateSessionState(TraceSession::State state); - - void OnAlert(const std::string& alert_name); + void CloseSession(); TraceManagerApp* const app_; const Config config_; + std::shared_ptr trace_controller_; uint32_t next_provider_id_ = 1u; - std::unique_ptr session_; std::list providers_; - std::queue alerts_; - std::queue watch_alert_callbacks_; async::Executor& executor_; TraceManager(const TraceManager&) = delete; @@ -82,6 +115,43 @@ class TraceManager : public controller::Controller, public provider::Registry { TraceManager& operator=(TraceManager&&) = delete; }; +// TODO(b/42083286): Remove after the old trace Controller protocol is removed +// This is an implementations of the old trace controller FIDL protocol. It is a +// temporary internal implementation which will be removed once all tracing +// clients are migrated to use the new protocols. +class OldTraceManager : public controller::Controller { + public: + OldTraceManager(TraceManagerApp* app, TraceManager* trace_manager, async::Executor& executor); + ~OldTraceManager() override; + + // For testing. + TraceSession* session() const; + + void OnEmptyControllerSet(); + + private: + // |Controller| implementation. + void InitializeTracing(controller::TraceConfig config, zx::socket output) override; + void TerminateTracing(controller::TerminateOptions options, TerminateTracingCallback cb) override; + void StartTracing(controller::StartOptions options, StartTracingCallback cb) override; + void StopTracing(controller::StopOptions options, StopTracingCallback cb) override; + void GetProviders(GetProvidersCallback cb) override; + void GetKnownCategories(GetKnownCategoriesCallback callback) override; + void WatchAlert(WatchAlertCallback cb) override; + void handle_unknown_method(uint64_t ordinal, bool method_has_response) override; + + void SendSessionStateEvent(controller::SessionState state); + controller::SessionState TranslateSessionState(TraceSession::State state); + + void OnAlert(const std::string& alert_name); + + controller::TerminateResult terminate_result_; + TraceManagerApp* const app_; + TraceManager* const trace_manager_; + fidl::InterfacePtr trace_controller_; + async::Executor& executor_; +}; + } // namespace tracing #endif // SRC_PERFORMANCE_TRACE_MANAGER_TRACE_MANAGER_H_ diff --git a/src/performance/trace_manager/trace_session.cc b/src/performance/trace_manager/trace_session.cc index 7eca81cc6b19..652ab8196093 100644 --- a/src/performance/trace_manager/trace_session.cc +++ b/src/performance/trace_manager/trace_session.cc @@ -117,8 +117,7 @@ void TraceSession::AddProvider(TraceProviderBundle* provider) { void TraceSession::MarkInitialized() { TransitionToState(State::kInitialized); } -void TraceSession::Terminate( - fit::function callback) { +void TraceSession::Terminate(fit::closure callback) { if (state_ == State::kTerminating) { FX_LOGS(DEBUG) << "Ignoring terminate request, already terminating"; return; @@ -137,7 +136,7 @@ void TraceSession::Terminate( void TraceSession::Start(fuchsia::tracing::BufferDisposition buffer_disposition, const std::vector& additional_categories, - controller::Controller::StartTracingCallback callback) { + controller::Session::StartTracingCallback callback) { FX_DCHECK(state_ == State::kInitialized || state_ == State::kStopped); if (force_clear_buffer_contents_) { @@ -168,7 +167,7 @@ void TraceSession::Start(fuchsia::tracing::BufferDisposition buffer_disposition, } void TraceSession::Stop(bool write_results, - fit::function callback) { + fit::function callback) { FX_DCHECK(state_ == State::kInitialized || state_ == State::kStarting || state_ == State::kStarted); @@ -249,8 +248,8 @@ void TraceSession::NotifyStarted() { FX_LOGS(DEBUG) << "Marking session as having started"; session_start_timeout_.Cancel(); auto callback = std::move(start_callback_); - controller::Controller_StartTracing_Result result; - controller::Controller_StartTracing_Response response; + controller::Session_StartTracing_Result result; + controller::Session_StartTracing_Response response; result.set_response(response); callback(std::move(result)); } @@ -309,9 +308,18 @@ void TraceSession::NotifyStopped() { if (stop_callback_) { FX_LOGS(DEBUG) << "Marking session as having stopped"; session_stop_timeout_.Cancel(); - controller::Controller_StopTracing_Result stop_result; - controller::Controller_StopTracing_Response response; - stop_result.set_response(response); + for (auto& tracee : tracees_) { + if (auto trace_stat = tracee->GetStats(); trace_stat.has_value()) { + trace_stats_.push_back(std::move(trace_stat.value())); + } else { + FX_LOGS(WARNING) << "No stats generated for " << tracee->bundle(); + } + } + + controller::Session_StopTracing_Result stop_result; + controller::TerminateResult result; + result.set_provider_stats(std::move(trace_stats_)); + stop_result.set_response(controller::Session_StopTracing_Response(std::move(result))); auto callback = std::move(stop_callback_); FX_DCHECK(callback); callback(std::move(stop_result)); @@ -347,11 +355,6 @@ void TraceSession::OnProviderTerminated(TraceProviderBundle* bundle) { } } } - if (auto trace_stat = (*it)->GetStats(); trace_stat.has_value()) { - trace_stats_.push_back(std::move(trace_stat.value())); - } else { - FX_LOGS(WARNING) << "No stats generated for " << *bundle; - } tracees_.erase(it); } @@ -374,15 +377,9 @@ void TraceSession::TerminateSessionIfEmpty() { FX_LOGS(DEBUG) << "Marking session as terminated, no more tracees"; session_terminate_timeout_.Cancel(); - - controller::Controller_TerminateTracing_Result result; - controller::TerminateResult terminate_result; - terminate_result.set_provider_stats(std::move(trace_stats_)); - result.set_response( - controller::Controller_TerminateTracing_Response(std::move(terminate_result))); auto callback = std::move(terminate_callback_); FX_DCHECK(callback); - callback(std::move(result)); + callback(); } } @@ -396,22 +393,11 @@ void TraceSession::FinishTerminatingDueToTimeout() { if (tracee->state() != Tracee::State::kTerminated) { FX_LOGS(WARNING) << "Timed out waiting for trace provider " << tracee->bundle() << " to terminate"; - } else { - if (auto trace_stat = tracee->GetStats(); trace_stat.has_value()) { - trace_stats_.push_back(std::move(trace_stat.value())); - } else { - FX_LOGS(WARNING) << "No stats generated for " << tracee->bundle(); - } } } - controller::Controller_TerminateTracing_Result result; - controller::TerminateResult terminate_result; - terminate_result.set_provider_stats(std::move(trace_stats_)); - result.set_response( - controller::Controller_TerminateTracing_Response(std::move(terminate_result))); auto callback = std::move(terminate_callback_); FX_DCHECK(callback); - callback(std::move(result)); + callback(); } } @@ -467,6 +453,15 @@ bool TraceSession::WriteProviderData(Tracee* tracee) { void TraceSession::Abort() { FX_LOGS(DEBUG) << "Fatal error occurred, aborting session"; write_results_on_terminate_ = false; + if (stop_callback_) { + TransitionToState(State::kStopped); + session_stop_timeout_.Cancel(); + controller::Session_StopTracing_Result stop_result; + stop_result.set_err(controller::StopErrorCode::ABORTED); + auto callback = std::move(stop_callback_); + FX_DCHECK(callback); + callback(std::move(stop_result)); + } abort_handler_(); } diff --git a/src/performance/trace_manager/trace_session.h b/src/performance/trace_manager/trace_session.h index 0485f4fff4d2..367b2c4922b1 100644 --- a/src/performance/trace_manager/trace_session.h +++ b/src/performance/trace_manager/trace_session.h @@ -95,14 +95,14 @@ class TraceSession { // Stops tracing first if necessary (see |Stop()|). // If terminating providers takes longer than |stop_timeout_|, we forcefully // terminate tracing and invoke |callback|. - void Terminate(fit::function callback); + void Terminate(fit::closure callback); // Starts the trace. // Invokes |callback| when all providers in this session have // acknowledged the start request, or after |start_timeout_| has elapsed. void Start(fuchsia::tracing::BufferDisposition buffer_disposition, const std::vector& additional_categories, - controller::Controller::StartTracingCallback callback); + controller::Session::StartTracingCallback callback); // Stops all providers that are part of this session, streams out // all remaining trace records and finally invokes |callback|. @@ -113,7 +113,7 @@ class TraceSession { // If stopping providers takes longer than |stop_timeout_|, we forcefully // stop tracing and invoke |callback|. void Stop(bool write_results, - fit::function callback); + fit::function callback); // Remove |provider|, it's dead Jim. void RemoveDeadProvider(TraceProviderBundle* provider); @@ -181,9 +181,9 @@ class TraceSession { async::TaskMethod session_terminate_timeout_{this}; - controller::Controller::StartTracingCallback start_callback_; - fit::function stop_callback_; - fit::function terminate_callback_; + controller::Session::StartTracingCallback start_callback_; + fit::function stop_callback_; + fit::closure terminate_callback_; fit::closure abort_handler_; AlertCallback alert_callback_; diff --git a/src/performance/trace_manager/tracee.cc b/src/performance/trace_manager/tracee.cc index d2aedd49ce3b..76c127aef93c 100644 --- a/src/performance/trace_manager/tracee.cc +++ b/src/performance/trace_manager/tracee.cc @@ -439,7 +439,7 @@ TransferStatus Tracee::TransferRecords() const { } std::optional Tracee::GetStats() const { - if (state_ == State::kTerminated) { + if (state_ == State::kTerminated || state_ == State::kStopped) { return std::move(provider_stats_); } return std::nullopt;