tp: add support for adding key-value metadata fields to summary

Also change how the summarization API looks to better support this
feature.

Change-Id: I26050b0c342eb6580ec35305e213671949ddd48a

include/perfetto/trace_processor/basic_types.h

@@ -22,6 +22,7 @@
 #include <cstddef>
 #include <cstdint>
 
+#include <optional>
 #include <string>
 #include <unordered_map>
 #include <utility>
@@ -306,12 +307,32 @@ struct SqlPackage {
   bool allow_override = false;
 };
 
-// The file format of the output returned from the trace summary functions.
-enum class TraceSummaryOutputFormat : uint8_t {
-  // Indicates that the ouput is `TraceSummary` encoded as a binary protobuf.
-  kBinaryProto,
-  // Indicates that the ouput is `TraceSummary` encoded as a text protobuf.
-  kTextProto,
+// Struct which defines how the trace should be summarized by
+// `TraceProcessor::Summarize`.
+struct TraceSummaryComputationSpec {
+  // The set of metric ids which should be computed and returned in the
+  // `TraceSummary` proto.
+  std::vector<std::string> v2_metric_ids;
+
+  // The id of the query (which must exist in the `query` field of one of the
+  // TraceSummary specs) which will be used to populate the `metadata` field of
+  // the TraceSummary proto. This query *must* output exactly two string columns
+  // `key` and `value` which will be used to populate the `metadata` field of
+  // the output proto.
+  std::optional<std::string> metadata_query_id;
+};
+
+// A struct which defines the how the TraceSummary output proto should be
+// formatted.
+struct TraceSummaryOutputSpec {
+  // The file format of the output returned from the trace summary functions.
+  enum class Format : uint8_t {
+    // Indicates that the ouput is `TraceSummary` encoded as a binary protobuf.
+    kBinaryProto,
+    // Indicates that the ouput is `TraceSummary` encoded as a text protobuf.
+    kTextProto,
+  };
+  Format format;
 };
 
 // A struct wrapping the bytes of a `TraceSummarySpec` instance.

include/perfetto/trace_processor/trace_processor.h

@@ -75,41 +75,40 @@ class PERFETTO_EXPORT_COMPONENT TraceProcessor : public TraceProcessorStorage {
   // |        Trace summary related functionality starts here        |
   // =================================================================
 
-  // Computes the v2 metrics (if non-empty, only the ones given by `metric_ids`)
-  // on the currently loaded trace based on the metric specifications inside
-  // `specs`.
+  // Creates a summary of the trace as defined by the `computation` and `specs`
+  // parameters.
+  //
+  // `computation` is a `TraceSummaryComputationSpec` struct which decides how
+  // the trace should be summarized. It does not contain any business logic
+  // itself, instead just referencing the contents of `specs`.
   //
   // Each entry in `specs` should point to an instance of the `TraceSummarySpec`
   // proto with `spec_format` defining the file format of each specs. This
   // function accepts a vector to make it easy to compute metrics in the common
   // case of having many different `TraceSummarySpec` files, each with a subset
-  // of the metrics which need to be computed.
+  // of the summary to be computed (e.g. metrics shareded across multiple
+  // files).
   //
-  // The result of computing the metrics will be returned in `output` (with a
-  // schema specified by the `TraceSummary` proto) with `output_format` defining
+  // The result of computing the summary will be returned in `output` (with a
+  // schema specified by the `TraceSummary` proto) with `output_spec` defining
   // the format that the data should be returned in.
   //
-  // `metric_ids` is an optional parameter which controls which metric ids from
-  // the specs should be computed. If empty, *all* metrics in all provided specs
-  // will be computed. If specified, every id must match the id of a metric in
-  // the spec or an error will be returned.
-  //
-  // Note: this function will only consider the `metric_spec` and `shared_query`
-  // fields of all the provided specs: any other fields will be silently
-  // ignored.
-  //
-  // Note: this function will only populate the `metric` field of the output,
-  // any other fields are guaranteed to be empty.
+  // Conceptual note: this function is designed with a split in `computation`
+  // vs `specs` is to allow for `specs` to be stored as self-contained set of
+  // protos on the filesystem or in a git repo which are then referenced by the
+  // embedder of trace processor to actually decide which parts of the spec
+  // matter for a particular trace. This allows decoupling what should be
+  // computed from how that computation should happen.
   //
   // Implementation note: after this function returns, any or all of the
   // referenced PerfettoSQL modules in any computed metrics will remain
   // included. This behaviour is *not* considered part of the API and should not
-  // be relied on. It is possible this will change in the future.
-  virtual base::Status ComputeV2Metrics(
+  // be relied on. It is likely this will change in the future.
+  virtual base::Status Summarize(
+      const TraceSummaryComputationSpec& computation,
       const std::vector<TraceSummarySpecBytes>& specs,
       std::vector<uint8_t>* output,
-      TraceSummaryOutputFormat output_format,
-      const std::vector<std::string>& metric_ids = {}) = 0;
+      const TraceSummaryOutputSpec& output_spec) = 0;
 
   // =================================================================
   // |        Metatracing related functionality starts here          |

protos/perfetto/trace_summary/file.proto

@@ -35,13 +35,14 @@ import "protos/perfetto/trace_summary/v2_metric.proto";
 message TraceSummarySpec {
   // The specification of a v2 trace metric.
   //
-  // See documentation for TraceMetricV2Config for usage of this field.
+  // See documentation for TraceMetricV2Spec for usage of this field.
   repeated TraceMetricV2Spec metric_spec = 1;
 
-  // Instances of structured queries whose ids can be referenced by the
-  // `inner_query_id` field of any `metric_spec.query`. Useful for sharing
-  // a single query across many different metrics.
-  repeated PerfettoSqlStructuredQuery shared_query = 2;
+  // Instances of structured queries whose ids can be referenced by:
+  // 1) `inner_query_id` field of any `metric_spec.query` which is useful for
+  //    sharing a single query across many different metrics.
+  // 2) computation of trace-wide metadata.
+  repeated PerfettoSqlStructuredQuery query = 2;
 }
 
 // Wrapper proto containing a bunch of outputs protos produced when computing
@@ -54,4 +55,14 @@ message TraceSummary {
   //
   // See documentation for TraceMetric for usage of this field.
   repeated TraceMetricV2 metric = 1;
+
+  // Metadata about the whole trace file as key value pairs. This is intended
+  // to put metadata about the trace (e.g. the device trace was collected on,
+  // the OS version) which gives context to other parts of the summary (e.g. the
+  // metrics).
+  message Metadata {
+    optional string key = 1;
+    optional string value = 2;
+  }
+  repeated Metadata metadata = 2;
 }

python/generators/diff_tests/runner.py

@@ -265,9 +265,10 @@ def __run_metrics_v2_test(
           '--extra-checks',
           '--perf-file',
           tmp_perf_file.name,
+          '--summary',
           '--summary-spec',
           tmp_spec_file.name,
-          '--compute-metrics-v2',
+          '--summary-metrics-v2',
           spec_message.metric_spec[0].id,
           '--summary-format',
           'binary',

src/trace_processor/BUILD.gn

@@ -191,6 +191,7 @@ if (enable_perfetto_trace_processor_sqlite) {
       "storage",
       "tables",
       "trace_summary",
+      "trace_summary:gen_cc_trace_summary_descriptor",
       "types",
       "util",
       "util:gzip",

src/trace_processor/perfetto_sql/generator/structured_query_generator.cc

@@ -19,6 +19,7 @@
 #include <algorithm>
 #include <cstddef>
 #include <cstdint>
+#include <cstring>
 #include <memory>
 #include <optional>
 #include <string>
@@ -68,17 +69,17 @@ struct QueryState {
   std::string sql;
 };
 
-using SharedQuery = StructuredQueryGenerator::SharedQuery;
-using SharedQueryProto = StructuredQueryGenerator::SharedQueryProto;
+using Query = StructuredQueryGenerator::Query;
+using QueryProto = StructuredQueryGenerator::QueryProto;
 
 class GeneratorImpl {
  public:
-  GeneratorImpl(base::FlatHashMap<std::string, SharedQueryProto>& shared_protos,
-                std::vector<SharedQuery>& shared,
+  GeneratorImpl(base::FlatHashMap<std::string, QueryProto>& protos,
+                std::vector<Query>& queries,
                 base::FlatHashMap<std::string, std::nullptr_t>& modules,
                 std::vector<std::string>& preambles)
-      : shared_queries_protos_(shared_protos),
-        shared_queries_(shared),
+      : query_protos_(protos),
+        queries_(queries),
         referenced_modules_(modules),
         preambles_(preambles) {}
 
@@ -113,7 +114,7 @@ class GeneratorImpl {
   static base::StatusOr<std::string> SelectColumnsAggregates(
       RepeatedString group_by,
       RepeatedProto aggregates,
-      RepeatedProto select_columns);
+      RepeatedProto select_cols);
   static base::StatusOr<std::string> SelectColumnsNoAggregates(
       RepeatedProto select_columns);
 
@@ -127,8 +128,8 @@ class GeneratorImpl {
   // Index of the current query we are processing in the `state_` vector.
   size_t state_index_ = 0;
   std::vector<QueryState> state_;
-  base::FlatHashMap<std::string, SharedQueryProto>& shared_queries_protos_;
-  std::vector<SharedQuery>& shared_queries_;
+  base::FlatHashMap<std::string, QueryProto>& query_protos_;
+  std::vector<Query>& queries_;
   base::FlatHashMap<std::string, std::nullptr_t>& referenced_modules_;
   std::vector<std::string>& preambles_;
 };
@@ -150,8 +151,8 @@ base::StatusOr<std::string> GeneratorImpl::Generate(
   for (size_t i = 0; i < state_.size(); ++i) {
     QueryState& state = state_[state_.size() - i - 1];
     if (state.type == QueryType::kShared) {
-      shared_queries_.emplace_back(SharedQuery{state.id_from_proto.value(),
-                                               state.table_name, state.sql});
+      queries_.emplace_back(
+          Query{state.id_from_proto.value(), state.table_name, state.sql});
       continue;
     }
     sql += state.table_name + " AS (" + state.sql + ")";
@@ -325,13 +326,13 @@ base::StatusOr<std::string> GeneratorImpl::IntervalIntersect(
 base::StatusOr<std::string> GeneratorImpl::ReferencedSharedQuery(
     protozero::ConstChars raw_id) {
   std::string id = raw_id.ToStdString();
-  auto* it = shared_queries_protos_.Find(id);
+  auto* it = query_protos_.Find(id);
   if (!it) {
     return base::ErrStatus("Shared query with id '%s' not found", id.c_str());
   }
-  auto sq = std::find_if(shared_queries_.begin(), shared_queries_.end(),
-                         [&](const SharedQuery& sq) { return id == sq.id; });
-  if (sq != shared_queries_.end()) {
+  auto sq = std::find_if(queries_.begin(), queries_.end(),
+                         [&](const Query& sq) { return id == sq.id; });
+  if (sq != queries_.end()) {
     return sq->table_name;
   }
   state_.emplace_back(QueryType::kShared,
@@ -550,15 +551,24 @@ base::StatusOr<std::string> GeneratorImpl::AggregateToString(
 base::StatusOr<std::string> StructuredQueryGenerator::Generate(
     const uint8_t* data,
     size_t size) {
-  GeneratorImpl impl(shared_queries_protos_, referenced_shared_queries_,
-                     referenced_modules_, preambles_);
+  GeneratorImpl impl(query_protos_, referenced_queries_, referenced_modules_,
+                     preambles_);
   ASSIGN_OR_RETURN(std::string sql,
                    impl.Generate(protozero::ConstBytes{data, size}));
   return sql;
 }
 
-base::Status StructuredQueryGenerator::AddSharedQuery(const uint8_t* data,
-                                                      size_t size) {
+base::StatusOr<std::string> StructuredQueryGenerator::GenerateById(
+    const std::string& id) {
+  auto* ptr = query_protos_.Find(id);
+  if (!ptr) {
+    return base::ErrStatus("Query with id %s not found", id.c_str());
+  }
+  return Generate(ptr->data.get(), ptr->size);
+}
+
+base::Status StructuredQueryGenerator::AddQuery(const uint8_t* data,
+                                                size_t size) {
   protozero::ProtoDecoder decoder(data, size);
   auto field = decoder.FindField(
       protos::pbzero::PerfettoSqlStructuredQuery::kIdFieldNumber);
@@ -569,8 +579,9 @@ base::Status StructuredQueryGenerator::AddSharedQuery(const uint8_t* data,
   }
   std::string id = field.as_std_string();
   auto ptr = std::make_unique<uint8_t[]>(size);
+  memcpy(ptr.get(), data, size);
   auto [it, inserted] =
-      shared_queries_protos_.Insert(id, SharedQueryProto{std::move(ptr), size});
+      query_protos_.Insert(id, QueryProto{std::move(ptr), size});
   if (!inserted) {
     return base::ErrStatus("Multiple shared queries specified with the ids %s",
                            id.c_str());

src/trace_processor/perfetto_sql/generator/structured_query_generator.h

@@ -21,6 +21,7 @@
 #include <cstdint>
 #include <memory>
 #include <string>
+#include <string_view>
 #include <vector>
 
 #include "perfetto/base/status.h"
@@ -36,12 +37,12 @@ using StructuredQuery = protos::pbzero::PerfettoSqlStructuredQuery;
 // query with support for shared queries.
 class StructuredQueryGenerator {
  public:
-  struct SharedQuery {
+  struct Query {
     std::string id;
     std::string table_name;
     std::string sql;
   };
-  struct SharedQueryProto {
+  struct QueryProto {
     std::unique_ptr<uint8_t[]> data;
     size_t size;
   };
@@ -54,9 +55,15 @@ class StructuredQueryGenerator {
   // as a common table expression (CTE).
   base::StatusOr<std::string> Generate(const uint8_t* data, size_t size);
 
-  // Adds a shared query to the internal state to reference in all future
-  // calls to `Generate`.
-  base::Status AddSharedQuery(const uint8_t* data, size_t size);
+  // Generates an SQL query for a query with the given id. The query should have
+  // been added with `AddQuery`
+  //
+  // See `Generate` above for expectations of this function
+  base::StatusOr<std::string> GenerateById(const std::string& id);
+
+  // Adds a query to the internal state to reference in all future calls to
+  // `Generate*`.
+  base::Status AddQuery(const uint8_t* data, size_t size);
 
   // Computes all the PerfettoSQL modules referenced by any past calls to
   // `Generate` and `AddSharedQuery`.
@@ -72,13 +79,11 @@ class StructuredQueryGenerator {
 
   // Returns a summary of all the shared queries which have been referenced
   // by any past calls to `Generate`.
-  std::vector<SharedQuery> referenced_shared_queries() const {
-    return referenced_shared_queries_;
-  }
+  std::vector<Query> referenced_queries() const { return referenced_queries_; }
 
  private:
-  base::FlatHashMap<std::string, SharedQueryProto> shared_queries_protos_;
-  std::vector<SharedQuery> referenced_shared_queries_;
+  base::FlatHashMap<std::string, QueryProto> query_protos_;
+  std::vector<Query> referenced_queries_;
 
   // We don't have FlatHashSet so just (ab)use FlatHashMap by storing a noop
   // value.

src/trace_processor/trace_processor_impl.cc

@@ -128,6 +128,7 @@
 #include "src/trace_processor/trace_processor_storage_impl.h"
 #include "src/trace_processor/trace_reader_registry.h"
 #include "src/trace_processor/trace_summary/summary.h"
+#include "src/trace_processor/trace_summary/trace_summary.descriptor.h"
 #include "src/trace_processor/types/trace_processor_context.h"
 #include "src/trace_processor/util/descriptors.h"
 #include "src/trace_processor/util/gzip_utils.h"
@@ -498,6 +499,13 @@ TraceProcessorImpl::TraceProcessorImpl(const Config& cfg)
       kAllWebviewMetricsDescriptor.data(), kAllWebviewMetricsDescriptor.size(),
       skip_prefixes);
 
+  // Add the summary descriptor to the summary pool.
+  {
+    base::Status status = context_.descriptor_pool_->AddFromFileDescriptorSet(
+        kTraceSummaryDescriptor.data(), kTraceSummaryDescriptor.size());
+    PERFETTO_CHECK(status.ok());
+  }
+
   RegisterAdditionalModules(&context_);
   InitPerfettoSqlEngine();
 
@@ -634,12 +642,13 @@ base::Status TraceProcessorImpl::RegisterSqlModule(SqlModule module) {
 // |  Trace-based metrics (v2) related functionality starts here   |
 // =================================================================
 
-base::Status TraceProcessorImpl::ComputeV2Metrics(
+base::Status TraceProcessorImpl::Summarize(
+    const TraceSummaryComputationSpec& computation,
     const std::vector<TraceSummarySpecBytes>& specs,
     std::vector<uint8_t>* output,
-    TraceSummaryOutputFormat format,
-    const std::vector<std::string>& metric_ids) {
-  return summary::ComputeV2Metrics(this, specs, output, format, metric_ids);
+    const TraceSummaryOutputSpec& output_spec) {
+  return summary::Summarize(this, *context_.descriptor_pool_, computation,
+                            specs, output, output_spec);
 }
 
 // =================================================================
@@ -663,7 +672,7 @@ base::Status TraceProcessorImpl::AnalyzeStructuredQueries(
     ASSIGN_OR_RETURN(newAnalyzedSq.sql, sqg.Generate(sq.ptr, sq.size));
     newAnalyzedSq.modules = sqg.ComputeReferencedModules();
     newAnalyzedSq.preambles = sqg.ComputePreambles();
-    sqg.AddSharedQuery(sq.ptr, sq.size);
+    sqg.AddQuery(sq.ptr, sq.size);
     output->push_back(newAnalyzedSq);
   }
   return base::OkStatus();