Add support for single sample metrics.

content/browser/renderer_host/one_shot_metric_host.h

+// Copyright 2017 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.
+
+#ifndef CONTENT_BROWSER_RENDERER_HOST_ONE_SHOT_METRIC_HOST_H_
+#define CONTENT_BROWSER_RENDERER_HOST_ONE_SHOT_METRIC_HOST_H_
+
+#include "base/macros.h"
+#include "base/metrics/histogram_base.h"
+#include "content/common/content_export.h"
+#include "content/common/metrics.mojom.h"
+
+namespace content {
+
+class CONTENT_EXPORT OneShotMetricHost : public mojom::OneShotMetricHost {

[Alexei Svitkine (slow), 2017/02/22 22:13:18]

Hmm, I'm not a fan of the OneShot name. To me, that sounds like a name where the
metric is only logged once - but in this case that's not quite the semantics.
It's more that you're only keeping the final value.

Maybe ScopedSingleValueHistogram?

Also, it seems useful for this functionality to exist outside of a Mojo
interface. How about making a plain old scoped class in base/metrics for this
and making this mojo interface a wrapper for that?

Finally, it seems that the name should depend on the type of histogram being
used. In this case, it's a counts histograms - so maybe
ScopedSingleValueCountsHistogram? I could imagine us later adding ones for
linear/enums or sparse.

[DaleCurtis, 2017/02/22 22:41:12]

Given rockot@'s recommendations on a provide class how about:

SingleValueHistogramProvider which vends SingleValueHistogramImpl instances to a
remote client which holds a mojom::SingleValueHistogram. Do you have a
preference on Sample vs Value?

Yes, I am planning a piece in base/; I'll prototype something with the next
patch set as an example.

Since we'll have a provider class we can keep the SingleValueHistogram name and
have the provider return differing instances. I.e. CreateLinearHistogram(),
CreateSparseHistogram(), etc would be methods on the provider that return the
appropriate SingleValueHistogram object.

+ public:
+  static void Create(mojom::OneShotMetricHostRequest request);

[bcwhite, 2017/02/21 12:37:54]

Pass by const-ref.

[DaleCurtis, 2017/02/21 17:54:29]

These are movable types, so per latest advice (and other mojom implementations)
they are supposed to passed by value:

https://cs.chromium.org/chromium/src/mojo/public/cpp/bindings/interface_reque...

[bcwhite, 2017/02/21 19:06:01]

You accomplish nothing by passing the parameter by value only to move that copy.

Either pass it by const-reference (const type&) and copy it to the final
destination or pass it by moveable-reference (type&&) and move it to the final
destination.  Note that the latter will require you to call Create() with a
movable type, either the result of an expression or Create(std::move(x)).

[DaleCurtis, 2017/02/22 22:41:12]

Actually, this doesn't compile as const&, this method signature is specified
exactly by the mojo interface callback as by-value and not const&.

[bcwhite, 2017/02/23 12:13:02]

Ah, yes.  You can can change the signature in the "typemap" file by defining how
Mojo should use that class, something like

metrics.mojom.OneShotMetricHostRequest=base::WhateverClass::OneShotMetricHostRequest[move_only]",

to the type_mappings structure.  There are other options, too; see
https://www.chromium.org/developers/design-documents/mojo/type-mapping

+
+  OneShotMetricHost();
+  ~OneShotMetricHost() override;
+
+ private:
+  // mojom::OneShotMetricHost implementation.
+  void Initialize(const std::string& name,
+                  base::HistogramBase::Sample min,
+                  base::HistogramBase::Sample max,
+                  uint32_t bucket_count) override;
+  void SetSample(base::HistogramBase::Sample sample) override;
+
+  struct Metric {
+    std::string name_;
+    base::HistogramBase::Sample min_;
+    base::HistogramBase::Sample max_;
+    uint32_t bucket_count;
+  };
+  std::unique_ptr<Metric> metric_;

[bcwhite, 2017/02/21 12:37:54]

Can you make these inline member variables instead of pointers?

[DaleCurtis, 2017/02/21 17:55:03]

Could, but then I also need bool values for when they're set.

[bcwhite, 2017/02/21 19:06:01]

Yes.  Better two booleans than two managed pointer objects and the overhead of
doing new/delete on structures of a few bytes.

+  std::unique_ptr<base::HistogramBase::Sample> sample_;

[Alexei Svitkine (slow), 2017/02/22 22:13:18]

Sample is a int32_t  and for numeric histograms, we don't support samples that
are negative for example, so you can use -1 as a placeholder when not set.

You can also DCHECK on the sample param to make sure it's not negative.

+
+  DISALLOW_COPY_AND_ASSIGN(OneShotMetricHost);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_RENDERER_HOST_ONE_SHOT_METRIC_HOST_H_