Metrics provider for statistical stack profiler

components/metrics/proto/call_stack_profile.proto

+// Copyright 2015 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.
+
+// Call stack sample data for a given profiling session.
+
+syntax = "proto2";
+
+option optimize_for = LITE_RUNTIME;
+
+package metrics;
+
+// Next tag: 5
+message CallStackProfile {
+

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please replace this newline with documentation.

[Mike Wittman, 2015/03/18 00:54:28]

Done.

+  message Sample {
+    // The callstack. Sample.entries[0] represents the call on the top of the
+    // stack.
+    repeated CallStackEntry entry = 1;

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please leave a blank line after this one.

[Mike Wittman, 2015/03/18 00:54:27]

Done.

+    // Number of times this stack signature occurs.
+    optional int64 count = 2;
+  }
+
+  // The callstack and counts.
+  repeated Sample sample = 1;

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please leave a blank line here.

[Mike Wittman, 2015/03/18 00:54:27]

Done.

+  // List of module ids found in this sample.
+  repeated ModuleIdentifier module_id = 2;
+
+  // Duration of this profile.
+  optional int32 profile_duration_ms = 3;

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please leave a blank line here.

[Mike Wittman, 2015/03/18 00:54:28]

Done.

+  // Time between samples.
+  optional int32 sampling_period_ms = 4;
+}
+
+// Describes an entry in the callstack.
+message CallStackEntry {
+  // Instruction pointer subtracted by module base.
+  optional uint64 address = 1;

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please leave a blank line here.

[Mike Wittman, 2015/03/18 00:54:28]

Done.

+  // Index to the module identifier in |module_ids| of CallStackProfile.
+  optional int32 module_id_index = 2;
+}

[Ilya Sherman, 2015/03/17 04:54:35]

Why are some of the messages declared as inner message types, and others as
outer ones?  I think the general preference is to use inner message types where
possible.

[Mike Wittman, 2015/03/18 00:54:28]

In the server-side review (cr/87065567) Sample was the only proto requested to
be nested. Possibly the others may be reused?

[Ilya Sherman, 2015/03/19 00:10:19]

That's fair, but it doesn't look like the reviewer who made that suggestion was
particularly familiar with the other UMA .proto files.  I'm pretty sure that
every other .proto file defines only a single message in the outer namespace. 
Please follow that convention in this file as well.

[Ilya Sherman, 2015/03/19 02:01:16]

Bump.

[Mike Wittman, 2015/03/19 21:51:18]

Sorry, this slipped my mind. Made the messages internal.

+
+// Uniquely identifies a module.
+message ModuleIdentifier {
+  // A hash that uniquely identifies a particular program version with high
+  // probability. This is parsed from headers of the loaded module.
+  // For binaries generated by GNU tools:
+  //   Contents of the .note.gnu.build-id field.
+  // On Windows:
+  //   GUID + AGE in the debug image headers of a module.
+  optional bytes build_id = 1;

[Ilya Sherman, 2015/03/17 04:54:35]

nit: Please leave a blank line here.

[Ilya Sherman, 2015/03/17 04:54:35]

"bytes" fields are generally discouraged, same as string ones.  Do we really
need the flexibility that "bytes" provides here?

[Mike Wittman, 2015/03/18 00:54:28]

Done.

[Mike Wittman, 2015/03/18 00:54:28]

Yes. This was addressed in the server-side review:

"Windows might have a fixed convention, but there is not a fixed length for
build-id on linux.  There's not even a requirement that it be a multiple of 4-
or 8-bytes. If you want to support multiple platforms, you'll need a byte
array."

+  // MD5Sum Prefix of the module name. This is the same hashing scheme as used
+  // to hash UMA histogram names.
+  optional fixed64 name_md5_prefix = 2;
+}