Add support for single sample metrics.

Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests

Review-Url: https://codereview.chromium.org/2687583002
Cr-Commit-Position: refs/heads/master@{#469524}
Committed: https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf328fbc1a96b0

[DaleCurtis, 2017-02-08 00:55:43]

Description was changed from

==========
Add support for single sample histograms.

Single sample histograms offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

*** ROUGH SKETCH ONLY, NOT FOR SUBMISSION, NEEDS ASSISTANCE :| ***

BUG=???
TEST=???
==========

to

==========
Add support for single sample histograms.

Single sample histograms offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

*** ROUGH SKETCH ONLY, NOT FOR SUBMISSION, NEEDS ASSISTANCE :| ***

BUG=689751
TEST=???
==========

[DaleCurtis, 2017-02-08 00:58:04]

dalecurtis@chromium.org changed reviewers:
+ asvitkine@chromium.org, bcwhite@chromium.org

[DaleCurtis, 2017-02-08 00:58:05]

This is the (very) rough draft based on chat discussion with asvitkine@. I'm not
versed in the layering of the metrics components well enough to know if this is
the right direction.

Roughly it seems like we want a mechanism for indicating that a given histogram
is not finalized and thus should never be reported to UMA. Phrasing this in a
way that respects layering is not easy. I.e. base/ has no idea about UMA, it
just knows things get sent to a statistics recorder.

I've attached a bug to this CL, which we can use for general discussion. There
are open questions around a lot of things :)

- The persistent allocator isn't always available. How does
SubprocessMetricsProvider work in this case?

- Who/what should be responsible for ignoring unfinalized metrics? Is
SubprocessMetricsProvider the right insertion point?

- Is it possible to generate multiple unique HistogramBase objects for single
histogram name?

[bcwhite, 2017-02-08 13:55:17]

> Roughly it seems like we want a mechanism for indicating that a given
histogram
> is not finalized and thus should never be reported to UMA. Phrasing this in a
> way that respects layering is not easy. I.e. base/ has no idea about UMA, it
> just knows things get sent to a statistics recorder.

Your method of having an "unfinalized" flag is what first came to my mind, too.

Flags are copied when new histograms are created from old ones, like what is
done with histograms from subprocesses the first time they get merged into the
Browser's StatisticsRecorder, but they are not copied/checked when merging to
one that already exists.

The upshot is that clearing that flag anywhere but in the Browser's (merged)
copy will not work reliably... at least not without other code changes to do
something specific with this flag.  

You can see how a merge is done in
PersistentHistogramAllocator::MergeHistogramDeltaToStatisticsRecorder().  There
will be something similar in the older RPC transfer code.


But instead of a flag, how about this:

Define your new histogram as a subclass of HistogramBase but have it construct
only using local memory (nothing persistent) and not registered with the
StatisticsRecorder.  Then, upon destruction, call FactoryGet() on a real
histogram type (Histogram, LinearHistogram, SparseHistogram, etc.) and add the
local data to what is returned.

That will give the same histogram interface but keep your data from being
managed by the metrics code until it destructs.  It will be lost in a crash (no
persistence) but then it wouldn't be finalized in that case either.

This doesn't cover the auto-finalization upon process exit but you can cover
that with an AtExit handler or similar.


> I've attached a bug to this CL, which we can use for general discussion. There
> are open questions around a lot of things :)
> 
> - The persistent allocator isn't always available. How does
> SubprocessMetricsProvider work in this case?

It doesn't.  Metrics from subprocesses come in via RPC when persistence is not
enabled.


> - Who/what should be responsible for ignoring unfinalized metrics? Is
> SubprocessMetricsProvider the right insertion point?

HistogramSnapshotManager is what processes all the metrics for upload.  It also
merges metrics from multiple sources, though that doesn't mean that some haven't
been previously merged together into the local StatisticsRecorder.


> - Is it possible to generate multiple unique HistogramBase objects for single
> histogram name?

No.  Code exists that reuses existing histograms if you try to create a new one
with the same name.  When imported from subprocesses, they get merged.

[DaleCurtis, 2017-02-08 19:32:29]

On 2017/02/08 at 13:55:17, bcwhite wrote: 
> The upshot is that clearing that flag anywhere but in the Browser's (merged)
copy will not work reliably... at least not without other code changes to do
something specific with this flag.  

Yeah, this is what I was trying to avoid. I was hoping you would know some magic
I didn't :)

> 
> But instead of a flag, how about this:
> 
> Define your new histogram as a subclass of HistogramBase but have it construct
only using local memory (nothing persistent) and not registered with the
StatisticsRecorder.  Then, upon destruction, call FactoryGet() on a real
histogram type (Histogram, LinearHistogram, SparseHistogram, etc.) and add the
local data to what is returned.
> 
> That will give the same histogram interface but keep your data from being
managed by the metrics code until it destructs.  It will be lost in a crash (no
persistence) but then it wouldn't be finalized in that case either.

I like the concept, but I don't see the advantage to inheriting from
HistogramBase over a class which just uses a HistogramBase from FactoryGet at
destruction. Given the single-sample use-case it seems advantageous to not
expose the full HistogramBase interface and instead only a single method for
setting the sample value. 

> This doesn't cover the auto-finalization upon process exit but you can cover
that with an AtExit handler or similar.

This was my first idea, but I thought AtExit handlers were frowned upon. There's
very little usage in the code base and we go out of our way to mark singletons
as leaky when possible. Do metrics have precedent for using AtExit handlers?
Whatever we decide here should drive the design in any case as clients can
already implement destruction time metrics through the normal macro based
mechanisms and a class variable.

[bcwhite, 2017-02-08 20:47:51]

> > The upshot is that clearing that flag anywhere but in the Browser's (merged)
> copy will not work reliably... at least not without other code changes to do
> something specific with this flag.  
> 
> Yeah, this is what I was trying to avoid. I was hoping you would know some
magic
> I didn't :)

No such luck.  Sorry.  :-(


> > But instead of a flag, how about this:
> > 
> > Define your new histogram as a subclass of HistogramBase but have it
construct
> only using local memory (nothing persistent) and not registered with the
> StatisticsRecorder.  Then, upon destruction, call FactoryGet() on a real
> histogram type (Histogram, LinearHistogram, SparseHistogram, etc.) and add the
> local data to what is returned.
> > 
> > That will give the same histogram interface but keep your data from being
> managed by the metrics code until it destructs.  It will be lost in a crash
(no
> persistence) but then it wouldn't be finalized in that case either.
> 
> I like the concept, but I don't see the advantage to inheriting from
> HistogramBase over a class which just uses a HistogramBase from FactoryGet at
> destruction. Given the single-sample use-case it seems advantageous to not
> expose the full HistogramBase interface and instead only a single method for
> setting the sample value. 

I think deriving from HistogramBase is still a good idea so that the interface
remains consistent and you get some things for free, like AddTime() and
AddBoolean().

But I don't see it being a big thing either way.  You can't register it with the
StatisticsRecorder because it takes ownership and so can't be used with
stack/scoped variables.


> > This doesn't cover the auto-finalization upon process exit but you can cover
> that with an AtExit handler or similar.
> 
> This was my first idea, but I thought AtExit handlers were frowned upon.
There's
> very little usage in the code base and we go out of our way to mark singletons
> as leaky when possible. Do metrics have precedent for using AtExit handlers?
> Whatever we decide here should drive the design in any case as clients can
> already implement destruction time metrics through the normal macro based
> mechanisms and a class variable.

Metrics don't have any AtExit methods.  They're considered to live forever and
so are never destroyed.

I expect AtExit _is_ frowned upon but if you've got a legitimate use for it, not
just cleaning up something unnecessarily, I think that this is exactly what you
want.

[Alexei Svitkine (slow), 2017-02-08 20:51:59]

I don't think AtExit handlers would be run at all in the renderer
processes. There was a discussion about it on chromium-dev@.

On Wed, Feb 8, 2017 at 3:47 PM, <bcwhite@chromium.org> wrote:

> > > The upshot is that clearing that flag anywhere but in the Browser's
> (merged)
> > copy will not work reliably... at least not without other code changes
> to do
> > something specific with this flag.
> >
> > Yeah, this is what I was trying to avoid. I was hoping you would know
> some
> magic
> > I didn't :)
>
> No such luck. Sorry. :-(
>
>
> > > But instead of a flag, how about this:
> > >
> > > Define your new histogram as a subclass of HistogramBase but have it
> construct
> > only using local memory (nothing persistent) and not registered with the
> > StatisticsRecorder. Then, upon destruction, call FactoryGet() on a real
> > histogram type (Histogram, LinearHistogram, SparseHistogram, etc.) and
> add the
> > local data to what is returned.
> > >
> > > That will give the same histogram interface but keep your data from
> being
> > managed by the metrics code until it destructs. It will be lost in a
> crash
> (no
> > persistence) but then it wouldn't be finalized in that case either.
> >
> > I like the concept, but I don't see the advantage to inheriting from
> > HistogramBase over a class which just uses a HistogramBase from
> FactoryGet at
> > destruction. Given the single-sample use-case it seems advantageous to
> not
> > expose the full HistogramBase interface and instead only a single method
> for
> > setting the sample value.
>
> I think deriving from HistogramBase is still a good idea so that the
> interface
> remains consistent and you get some things for free, like AddTime() and
> AddBoolean().
>
> But I don't see it being a big thing either way. You can't register it
> with the
> StatisticsRecorder because it takes ownership and so can't be used with
> stack/scoped variables.
>
>
> > > This doesn't cover the auto-finalization upon process exit but you can
> cover
> > that with an AtExit handler or similar.
> >
> > This was my first idea, but I thought AtExit handlers were frowned upon.
> There's
> > very little usage in the code base and we go out of our way to mark
> singletons
> > as leaky when possible. Do metrics have precedent for using AtExit
> handlers?
> > Whatever we decide here should drive the design in any case as clients
> can
> > already implement destruction time metrics through the normal macro based
> > mechanisms and a class variable.
>
> Metrics don't have any AtExit methods. They're considered to live forever
> and
> so are never destroyed.
>
> I expect AtExit _is_ frowned upon but if you've got a legitimate use for
> it, not
> just cleaning up something unnecessarily, I think that this is exactly
> what you
> want.
>
>
> https://codereview.chromium.org/2687583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[bcwhite, 2017-02-08 21:15:40]

> I don't think AtExit handlers would be run at all in the renderer
> processes. There was a discussion about it on chromium-dev@.

Well that's a problem, then.

The SubprocessMetricsProvider is the only thing that would have a chance of
recovering that information.  You'd have to flag your histograms with whatever
value you define but NOT include the UMA flag.  When the SMP is running over the
data from an exited process (and only when it exited, not during periodic
collection), it would have to notice these and process them appropriately,
performing whatever adjustments are necessary and changing the flags before
merging them.

If you can assume "finalization" only at process exit then there is nothing for
your class to do on destruction which means you can just use existing histogram
classes with all the special logic implemented in the SMP.

The downside is that it can only be used in subprocesses and will only get
collected when the process exits, not during periodic collection.

This would also work only when PersistentHistograms are enabled but it's a 100%
experiment in M57 and above with every intention of remaining that way.

But if you want to "finalize" periodically...  that becomes a lot more
complicated.

[DaleCurtis, 2017-02-08 21:46:25]

I proposed this during chat with asvitkine@ and it's sounding better and better,
WDYT:

- Introduce helper class which reports to a mojo service hosted in the browser
process.
- Mojo service records UMA metric as soon as the channel closes through the
normal means; either through termination or standard destruction.

The tricky part is layering it in such a way that base/ doesn't pick up any mojo
dependencies; so we need an interface for the renderer side piece which talks to
a browser side mojo service. This is necessary since many renderer side clients
don't know about content/chrome/etc.

All said this should be very easy to implement; it is more of a shim on top of
the histogram system than being something built in, but maybe that's for the
best given our discussions so far.

[bcwhite, 2017-02-09 00:05:41]

> I proposed this during chat with asvitkine@ and it's sounding better and
better,
> WDYT:
> 
> - Introduce helper class which reports to a mojo service hosted in the browser
> process.
> - Mojo service records UMA metric as soon as the channel closes through the
> normal means; either through termination or standard destruction.
> 
> The tricky part is layering it in such a way that base/ doesn't pick up any
mojo
> dependencies; so we need an interface for the renderer side piece which talks
to
> a browser side mojo service. This is necessary since many renderer side
clients
> don't know about content/chrome/etc.
> 
> All said this should be very easy to implement; it is more of a shim on top of
> the histogram system than being something built in, but maybe that's for the
> best given our discussions so far.

I know very little about Mojo so can't comment on the effort involved.  I know
my stuff, so when I suggest using the SubprocessMetricsProvider, I may be
viewing your problem as a nail to my hammer.

I _can_ say that you should be able to create your own MetricsProvider that
provides upon request anything it has collected to that point directly to the
HistogramSnapshotManager that is performing an upload.

Or you can create your own HistogramProvider that merges upon request anything
it has collected into the StatisticsRecorder of the main Browser process.  This
has the advantage that the histograms will appear on the chrome://histograms
page.

Both of those seem like possibilities for your Mojo Browser-side implementation
and neither needs to be implemented in base/ or require any changes there-to.

[DaleCurtis, 2017-02-17 23:36:34]

Latest patch set shows what a mojo approach might look like. From here we'd add
some base/metrics/ shim that is setup per renderer process with the ability to
create the remote interfaces as needed. We can add creation helper macros to
ensure counts, etc don't drift from what are prescribed in the macros.h file
today.

We can unbind those interfaces and pass them around threads as required, but
unlike standard histograms these would be required to always be used from the
same thread.

The overhead is a little high per instance, but presumably these would be rarely
used.

[bcwhite, 2017-02-21 12:37:55]

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:17: static void
Create(mojom::OneShotMetricHostRequest request);
Pass by const-ref.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:36: std::unique_ptr<Metric>
metric_;
Can you make these inline member variables instead of pointers?

https://codereview.chromium.org/2687583002/diff/20001/content/renderer/media/...
File content/renderer/media/render_media_log.cc (right):

https://codereview.chromium.org/2687583002/diff/20001/content/renderer/media/...
content/renderer/media/render_media_log.cc:56: mojom::OneShotMetricHostPtr
one_shot_metric;
Why is the renderer accessing a "host" object?

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

Any high level feedback? Will rope in the mojo, content experts if this doesn't
seem crazy as a new metrics idiom.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:17: static void
Create(mojom::OneShotMetricHostRequest request);
On 2017/02/21 at 12:37:54, bcwhite wrote:
> Pass by const-ref.

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...

https://codereview.chromium.org/2687583002/diff/20001/content/renderer/media/...
File content/renderer/media/render_media_log.cc (right):

https://codereview.chromium.org/2687583002/diff/20001/content/renderer/media/...
content/renderer/media/render_media_log.cc:56: mojom::OneShotMetricHostPtr
one_shot_metric;
On 2017/02/21 at 12:37:54, bcwhite wrote:
> Why is the renderer accessing a "host" object?

Eh, it's just the naming scheme commonly used by mojo setups. I think XXXImpl is
also a common idiom that would make more sense here since each GetInterface()
call creates a new "host" side impl.

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

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:36: std::unique_ptr<Metric>
metric_;
On 2017/02/21 at 12:37:54, bcwhite wrote:
> Can you make these inline member variables instead of pointers?

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

[bcwhite, 2017-02-21 19:06:02]

At a high level...  I don't know enough about Mojo to evaluate it.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:17: static void
Create(mojom::OneShotMetricHostRequest request);
On 2017/02/21 17:54:29, DaleCurtis wrote:
> On 2017/02/21 at 12:37:54, bcwhite wrote:
> > Pass by const-ref.
> 
> 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...

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)).

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:36: std::unique_ptr<Metric>
metric_;
On 2017/02/21 17:55:03, DaleCurtis wrote:
> On 2017/02/21 at 12:37:54, bcwhite wrote:
> > Can you make these inline member variables instead of pointers?
> 
> Could, but then I also need bool values for when they're set.

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

[DaleCurtis, 2017-02-21 20:08:52]

dalecurtis@chromium.org changed reviewers:
+ rockot@chromium.org

[DaleCurtis, 2017-02-21 20:08:52]

Okay, +rockot for mojo commentary then. rockot: What do you think about using
mojo for the purpose of single-value histograms? See message #11 for next steps
details.

[Ken Rockot(use gerrit already), 2017-02-21 20:26:47]

On 2017/02/21 at 20:08:52, dalecurtis wrote:
> Okay, +rockot for mojo commentary then. rockot: What do you think about using
mojo for the purpose of single-value histograms? See message #11 for next steps
details.

Seems OK, though interfaces with explicit Initialize-style messages are somewhat
unfortunate (i.e. easier to misuse.) So a few high-level comments:

1. Consider breaking this into two interfaces:

  interface OneShotMetricProvider {
    Acquire(string name, int32 min, int32 max, uint32 bucket_count,
OneShotMetric& request);
  };

  interface OneShotMetric {
    SetSample(int32 sample);
  };

Then any given OneShotMetric pipe is exclusively associated with one and only
one metric for the extent of its lifetime.


2. It's worth at least considering how this might be used by other process types
in the future. I guess (as a non-content and non-metrics owner) I'm fine with
this arrangement as-is, but in general the practice of stuffing new interfaces
into RPHI's InterfaceRegistry bag does not scale well. I suppose this may
eventually belong in the resource_coordinator service, and client processes
would acquire the interface directly from that service.

[Ken Rockot(use gerrit already), 2017-02-21 20:27:15]

rockot@chromium.org changed reviewers:
+ oysteine@chromium.org

[Ken Rockot(use gerrit already), 2017-02-21 20:27:15]

and +oysteine in case he has any thoughts re my comment on resource_coordinator
in the previous reply

[oystein (OOO til 10th of July), 2017-02-21 21:36:18]

On 2017/02/21 at 20:27:15, rockot wrote:
> and +oysteine in case he has any thoughts re my comment on
resource_coordinator in the previous reply

Seems like a good fit to me; the current thinking is that the
resource_coordinator will evolve over time to be the central service for
receiving metrics for the purposes of coordinating schedulers etc but also for
logging (I think we'll end up merging in the tracing service, for example).

If the code here could be in /components rather than /content that'd make it
pretty trivial to move, too.

[DaleCurtis, 2017-02-21 21:40:34]

On 2017/02/21 at 21:36:18, oysteine wrote:
> On 2017/02/21 at 20:27:15, rockot wrote:
> > and +oysteine in case he has any thoughts re my comment on
resource_coordinator in the previous reply
> 
> Seems like a good fit to me; the current thinking is that the
resource_coordinator will evolve over time to be the central service for
receiving metrics for the purposes of coordinating schedulers etc but also for
logging (I think we'll end up merging in the tracing service, for example).
> 
> If the code here could be in /components rather than /content that'd make it
pretty trivial to move, too.

Yup, it can easily live there. I'd probably put it in the components/metrics
folder if that's okay with metrics folk.

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

Generally seems reasonable. Some comments below.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:15: class CONTENT_EXPORT
OneShotMetricHost : public mojom::OneShotMetricHost {
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.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:37:
std::unique_ptr<base::HistogramBase::Sample> sample_;
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.

[DaleCurtis, 2017-02-22 22:41:13]

Thanks for the feedback. Summing up:
- Rename to SingleValueHistogram (unless someone prefers SingleSampleHistogram).
- Have a SingleValueHistogramProvider which can vend SingleValueHistogram mojo
objects that are implemented by an SingleValueHistogramImpl.
- Move code to components/metrics/{single_value_histogram*}.{cc,h}
- Add base/ shim that clients can use without needing to be aware of content/,
mojo, etc.

I'll try to address this over the next couple days and ping when I have a new CL
up.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:15: class CONTENT_EXPORT
OneShotMetricHost : public mojom::OneShotMetricHost {
On 2017/02/22 at 22:13:18, Alexei Svitkine (slow) wrote:
> 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.

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.

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:17: static void
Create(mojom::OneShotMetricHostRequest request);
On 2017/02/21 at 19:06:01, bcwhite wrote:
> On 2017/02/21 17:54:29, DaleCurtis wrote:
> > On 2017/02/21 at 12:37:54, bcwhite wrote:
> > > Pass by const-ref.
> > 
> > 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...
> 
> 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)).

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]

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/one_shot_metric_host.h (right):

https://codereview.chromium.org/2687583002/diff/20001/content/browser/rendere...
content/browser/renderer_host/one_shot_metric_host.h:17: static void
Create(mojom::OneShotMetricHostRequest request);
On 2017/02/22 22:41:12, DaleCurtis wrote:
> On 2017/02/21 at 19:06:01, bcwhite wrote:
> > On 2017/02/21 17:54:29, DaleCurtis wrote:
> > > On 2017/02/21 at 12:37:54, bcwhite wrote:
> > > > Pass by const-ref.
> > > 
> > > 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...
> > 
> > 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)).
> 
> Actually, this doesn't compile as const&, this method signature is specified
> exactly by the mojo interface callback as by-value and not const&.

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

[DaleCurtis, 2017-04-20 23:27:27]

dalecurtis@chromium.org changed reviewers:
+ nick@chromium.org

[DaleCurtis, 2017-04-20 23:27:28]

Okay, this is ready for another design review pass. Sorry for delay, manager
perf O_o.

New in this patch set:
- OneShotMetric renamed to SingleValueCountsHistogram.

- base::SingleValueHistogramsFactory shim for non-content, non-component clients
per asvitkine@'s recommendation. Implementation lives in components/metrics. Did
not take Scoped prefix since these will always be vended as std::unique_ptr so
its kind of implicit.

- SingleValueHistogramsProvider interface introduced per rockot@'s
recommendation. Staged for additional histogram types in the future per
asvitkine@'s recommendation.
---- rockot@ please provide thread safety commentary on 
components/metrics/single_value_histograms_factory_impl.cc

- All code moved into components/metrics per oysteine@'s recommendation.
---- +nick@ as the content/ OWNER since this requires content/ to take a
components/metrics DEP and to initialize the base::SingleValueHistogramsFactory
during RenderThreadImpl::Init().

As before, example syntax is shown in content/renderer/media/render_media_log.cc

[Ken Rockot(use gerrit already), 2017-04-21 17:40:56]

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
File components/metrics/single_value_histograms_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
components/metrics/single_value_histograms_factory_impl.cc:38: // safe to access
from any thread since it is created and persists for the
No, InterfacePtr is not thread-safe.

[DaleCurtis, 2017-04-21 17:44:33]

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
File components/metrics/single_value_histograms_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
components/metrics/single_value_histograms_factory_impl.cc:38: // safe to access
from any thread since it is created and persists for the
On 2017/04/21 at 17:40:56, Ken Rockot wrote:
> No, InterfacePtr is not thread-safe.

Thanks, will redo this section to use task_runner, this will cause the Create()
function below to become async unfortunately. Are there any thread-safe mojo
concepts you'd recommend here vs the standard trampoline?

[Ken Rockot(use gerrit already), 2017-04-21 18:03:27]

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
File components/metrics/single_value_histograms_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
components/metrics/single_value_histograms_factory_impl.cc:38: // safe to access
from any thread since it is created and persists for the
On 2017/04/21 at 17:44:33, DaleCurtis wrote:
> On 2017/04/21 at 17:40:56, Ken Rockot wrote:
> > No, InterfacePtr is not thread-safe.
> 
> Thanks, will redo this section to use task_runner, this will cause the
Create() function below to become async unfortunately. Are there any thread-safe
mojo concepts you'd recommend here vs the standard trampoline?

We'd like a proper thread-safe InterfacePtr, but some of the semantics (e.g.
dispatching replies and error callbacks) are tricky and we don't have one yet.
We do have ThreadSafeInterfacePtr, but it does trampolining anyway and I would
strongly discourage its use in your case. You can just do your own trampolining
and it will likely be less messy.

Another option to consider would be to use thread-local InterfacePtrs. I think
that might be preferable. It's probably cheaper than trampolining.

[DaleCurtis, 2017-04-21 18:18:35]

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
File components/metrics/single_value_histograms_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/40001/components/metrics/sing...
components/metrics/single_value_histograms_factory_impl.cc:38: // safe to access
from any thread since it is created and persists for the
On 2017/04/21 at 18:03:27, Ken Rockot wrote:
> On 2017/04/21 at 17:44:33, DaleCurtis wrote:
> > On 2017/04/21 at 17:40:56, Ken Rockot wrote:
> > > No, InterfacePtr is not thread-safe.
> > 
> > Thanks, will redo this section to use task_runner, this will cause the
Create() function below to become async unfortunately. Are there any thread-safe
mojo concepts you'd recommend here vs the standard trampoline?
> 
> We'd like a proper thread-safe InterfacePtr, but some of the semantics (e.g.
dispatching replies and error callbacks) are tricky and we don't have one yet.
We do have ThreadSafeInterfacePtr, but it does trampolining anyway and I would
strongly discourage its use in your case. You can just do your own trampolining
and it will likely be less messy.
> 
> Another option to consider would be to use thread-local InterfacePtrs. I think
that might be preferable. It's probably cheaper than trampolining.

Can you elaborate on how a TL IP would work? I.e. are you suggesting we only
allow clients to use this from the creating thread? media/ consists of many
threads and the space of base/ clients even more, so the interface does need
thread-safety.

[Ken Rockot(use gerrit already), 2017-04-21 18:29:35]

Whenever any thread wants to use the interface, it graps the TLS
InterfacePtr. If said InterfacePtr is null, it creates a new pipe, stuffs
the Ptr end in TLS, and sends the request end over to some other thread to
be passed over IPC and eventually bound.

On Fri, Apr 21, 2017 at 11:18 AM, <dalecurtis@chromium.org> wrote:

>
> https://codereview.chromium.org/2687583002/diff/40001/
> components/metrics/single_value_histograms_factory_impl.cc
> File components/metrics/single_value_histograms_factory_impl.cc (right):
>
> https://codereview.chromium.org/2687583002/diff/40001/
> components/metrics/single_value_histograms_factory_impl.cc#newcode38
> components/metrics/single_value_histograms_factory_impl.cc:38: // safe
> to access from any thread since it is created and persists for the
> On 2017/04/21 at 18:03:27, Ken Rockot wrote:
> > On 2017/04/21 at 17:44:33, DaleCurtis wrote:
> > > On 2017/04/21 at 17:40:56, Ken Rockot wrote:
> > > > No, InterfacePtr is not thread-safe.
> > >
> > > Thanks, will redo this section to use task_runner, this will cause
> the Create() function below to become async unfortunately. Are there any
> thread-safe mojo concepts you'd recommend here vs the standard
> trampoline?
> >
> > We'd like a proper thread-safe InterfacePtr, but some of the semantics
> (e.g. dispatching replies and error callbacks) are tricky and we don't
> have one yet. We do have ThreadSafeInterfacePtr, but it does
> trampolining anyway and I would strongly discourage its use in your
> case. You can just do your own trampolining and it will likely be less
> messy.
> >
> > Another option to consider would be to use thread-local InterfacePtrs.
> I think that might be preferable. It's probably cheaper than
> trampolining.
>
> Can you elaborate on how a TL IP would work? I.e. are you suggesting we
> only allow clients to use this from the creating thread? media/ consists
> of many threads and the space of base/ clients even more, so the
> interface does need thread-safety.
>
> https://codereview.chromium.org/2687583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2017-04-21 18:53:21]

On 2017/04/21 at 18:29:35, rockot wrote:
> Whenever any thread wants to use the interface, it graps the TLS
> InterfacePtr. If said InterfacePtr is null, it creates a new pipe, stuffs
> the Ptr end in TLS, and sends the request end over to some other thread to
> be passed over IPC and eventually bound.
> 

Looking at the path to creating a new pipe, none of those classes seem
thread-safe either though. I.e., i'd need to create a whole new
ServiceManagerConnection() for the TLS pointer; is that the path you're
suggesting here or am I misreading the header documentation?

[Ken Rockot(use gerrit already), 2017-04-21 19:10:16]

Err, no you don't need to do that.

You should still maintain a TaskRunner from which a Connector can be used.
Then you write (psuedocode, assume you have a connector and task runner as
needed on the appropriate thread):

void BindFooOnCorrectThread(Connector* connector, FooRequest request) {
  connector->BindInterface(kServiceName, std::move(request));
}

// Callable on any thread.
FooPtr CreateFoo() {
  FooPtr foo;
  task_runner->PostTask(
      FROM_HERE,
      base::BindOnce(&BindFooOnCorrectThread,
                     connector, std::move(request)));
  return foo;
}

and you call CreateFoo() as needed.

On Fri, Apr 21, 2017 at 11:53 AM, <dalecurtis@chromium.org> wrote:

> On 2017/04/21 at 18:29:35, rockot wrote:
> > Whenever any thread wants to use the interface, it graps the TLS
> > InterfacePtr. If said InterfacePtr is null, it creates a new pipe, stuffs
> > the Ptr end in TLS, and sends the request end over to some other thread
> to
> > be passed over IPC and eventually bound.
> >
>
> Looking at the path to creating a new pipe, none of those classes seem
> thread-safe either though. I.e., i'd need to create a whole new
> ServiceManagerConnection() for the TLS pointer; is that the path you're
> suggesting here or am I misreading the header documentation?
>
> https://codereview.chromium.org/2687583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-04-21 19:19:39]

Oops. That should be:

task_runner->PostTask(
      FROM_HERE,
      base::BindOnce(&BindFooOnCorrectThread,
                     connector, mojo::MakeRequest(&foo)));

On Fri, Apr 21, 2017 at 12:10 PM, Ken Rockot <rockot@chromium.org> wrote:

> Err, no you don't need to do that.
>
> You should still maintain a TaskRunner from which a Connector can be used.
> Then you write (psuedocode, assume you have a connector and task runner as
> needed on the appropriate thread):
>
> void BindFooOnCorrectThread(Connector* connector, FooRequest request) {
>   connector->BindInterface(kServiceName, std::move(request));
> }
>
> // Callable on any thread.
> FooPtr CreateFoo() {
>   FooPtr foo;
>   task_runner->PostTask(
>       FROM_HERE,
>       base::BindOnce(&BindFooOnCorrectThread,
>                      connector, std::move(request)));
>   return foo;
> }
>
> and you call CreateFoo() as needed.
>
> On Fri, Apr 21, 2017 at 11:53 AM, <dalecurtis@chromium.org> wrote:
>
>> On 2017/04/21 at 18:29:35, rockot wrote:
>> > Whenever any thread wants to use the interface, it graps the TLS
>> > InterfacePtr. If said InterfacePtr is null, it creates a new pipe,
>> stuffs
>> > the Ptr end in TLS, and sends the request end over to some other thread
>> to
>> > be passed over IPC and eventually bound.
>> >
>>
>> Looking at the path to creating a new pipe, none of those classes seem
>> thread-safe either though. I.e., i'd need to create a whole new
>> ServiceManagerConnection() for the TLS pointer; is that the path you're
>> suggesting here or am I misreading the header documentation?
>>
>> https://codereview.chromium.org/2687583002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2017-04-21 19:20:22]

On 2017/04/21 at 19:10:16, rockot wrote:
> Err, no you don't need to do that.
> 
> You should still maintain a TaskRunner from which a Connector can be used.
> Then you write (psuedocode, assume you have a connector and task runner as
> needed on the appropriate thread):

Thanks! That's much clearer, I thought you we're implying I'd Clone() or
recreate the connector. I'll rework to use this approach.

[DaleCurtis, 2017-04-24 23:42:27]

Patchset #4 (id:60001) has been deleted

[DaleCurtis, 2017-04-24 23:48:53]

Okay latest patch set incorporates rockot@'s feedback; before I get started on
tests and such, do others have any further high level feedback? nick@,
asvitkine@ especially :)

[DaleCurtis, 2017-04-24 23:49:11]

Description was changed from

==========
Add support for single sample histograms.

Single sample histograms offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

*** ROUGH SKETCH ONLY, NOT FOR SUBMISSION, NEEDS ASSISTANCE :| ***

BUG=689751
TEST=???
==========

to

==========
Add support for single sample histograms.

Single sample histograms offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

BUG=689751
TEST=???
==========

[Alexei Svitkine (slow), 2017-04-26 18:12:53]

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
File base/metrics/single_value_histograms.cc (right):

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
base/metrics/single_value_histograms.cc:12: // we'd need all
run_all_unittests.cc to set this up, which is annoying...
Can we have a default factor that creates instances that do local logging on
dtor?

The mojo interface is a nice to have on top of that, with the main benefit of
not losing metrics when there's a crash/unclean termination.

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
File base/metrics/single_value_histograms.h (right):

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
base/metrics/single_value_histograms.h:16: class BASE_EXPORT
SingleValueCountsHistogram {
The convention we use now in base/metrics/histogram_macros.h is for counts
histograms to actually specify their max in the name. So instead of
UMA_HISTOGRAM_COUNTS() we prefer UMA_HISTOGRAM_COUNTS_1M().

However, this interface doesn't seem to be specific to a given type of histogram
- e.g. the same interface can be used for different counts or even sparse/enum
histograms. So I suggest removing Counts from the name.

However, one point of confusion is calling this object a Histogram - which is
already used for other classes that have a different purpose. Maybe
HistogramLogger would be a better name? e.g. SingleValueHistogramLogger?

The comments should also make it clear that it's a single value per instance of
this object, and not ever - e.g. you can still use this to log things many
times, via different instances.

https://codereview.chromium.org/2687583002/diff/80001/components/metrics/DEPS
File components/metrics/DEPS (right):

https://codereview.chromium.org/2687583002/diff/80001/components/metrics/DEPS...
components/metrics/DEPS:11: "+content/public/common",
We don't want metrics component to depend on content. In particular, because it
may be used by things that don't depend on content themselves, like iOS Chrome.

The convention we use inside components/metrics is to have a sub-directory for
things that have more dependencies - e.g. components/metrics/net has code that
has dependencies on net/. So it would make sense to add the new stuff under
components/metrics/content if it depends on content.

[DaleCurtis, 2017-04-29 00:23:25]

Description was changed from

==========
Add support for single sample histograms.

Single sample histograms offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

BUG=689751
TEST=???
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

BUG=689751
TEST=unittests tbd
==========

[DaleCurtis, 2017-04-29 00:30:35]

Okay, still missing tests, but the latest patch set addresses discussions with
nick@ and asvitkine@ today:
- Naming is now SingleSampleMetric.
- Factory has methods like CreateCustomCountsMetric() or (in the future)
CreateCounts100Metric().
- Default implementation is provided for unittests.
- Removes content/ DEP from components metrics/

Nick also suggested that a future CL consider adding some macros to keep it
familiar for folks. E.g. classes would contain a
std::unique_ptr<SingleSampleMetric> sample_ member variable and we'd have macros
like UMA_SINGLE_SAMPLE_HISTOGRAM_CUSTOM_COUNTS(sample_, "a.b.c", 1, 2, 3, ) --
preserving the "name exists only in one place" behavior we get today.

I don't think we need to resolve that in this CL, but is something we can
discuss for a followup.

Will add tests and ping again next week. Thanks!

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
File base/metrics/single_value_histograms.cc (right):

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
base/metrics/single_value_histograms.cc:12: // we'd need all
run_all_unittests.cc to set this up, which is annoying...
On 2017/04/26 at 18:12:53, Alexei Svitkine (slow) wrote:
> Can we have a default factor that creates instances that do local logging on
dtor?
> 
> The mojo interface is a nice to have on top of that, with the main benefit of
not losing metrics when there's a crash/unclean termination.

Done; this also allows reuse of the default implementation for the mojo portion.

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
File base/metrics/single_value_histograms.h (right):

https://codereview.chromium.org/2687583002/diff/80001/base/metrics/single_val...
base/metrics/single_value_histograms.h:16: class BASE_EXPORT
SingleValueCountsHistogram {
On 2017/04/26 at 18:12:53, Alexei Svitkine (slow) wrote:
> The convention we use now in base/metrics/histogram_macros.h is for counts
histograms to actually specify their max in the name. So instead of
UMA_HISTOGRAM_COUNTS() we prefer UMA_HISTOGRAM_COUNTS_1M().
> 
> However, this interface doesn't seem to be specific to a given type of
histogram - e.g. the same interface can be used for different counts or even
sparse/enum histograms. So I suggest removing Counts from the name.
> 
> However, one point of confusion is calling this object a Histogram - which is
already used for other classes that have a different purpose. Maybe
HistogramLogger would be a better name? e.g. SingleValueHistogramLogger?
> 
> The comments should also make it clear that it's a single value per instance
of this object, and not ever - e.g. you can still use this to log things many
times, via different instances.

Done.

https://codereview.chromium.org/2687583002/diff/80001/components/metrics/DEPS
File components/metrics/DEPS (right):

https://codereview.chromium.org/2687583002/diff/80001/components/metrics/DEPS...
components/metrics/DEPS:11: "+content/public/common",
On 2017/04/26 at 18:12:53, Alexei Svitkine (slow) wrote:
> We don't want metrics component to depend on content. In particular, because
it may be used by things that don't depend on content themselves, like iOS
Chrome.
> 
> The convention we use inside components/metrics is to have a sub-directory for
things that have more dependencies - e.g. components/metrics/net has code that
has dependencies on net/. So it would make sense to add the new stuff under
components/metrics/content if it depends on content.

Done. Now the service name is passed in as a std::string, so no DEP necessary.

[DaleCurtis, 2017-04-29 00:32:44]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

BUG=689751
TEST=unittests tbd
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:

  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);

Only the last sample would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

[DaleCurtis, 2017-04-29 00:33:05]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:

  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);

Only the last sample would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

[DaleCurtis, 2017-04-29 00:33:16]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

[DaleCurtis, 2017-05-03 00:11:14]

Patchset #6 (id:120001) has been deleted

[DaleCurtis, 2017-05-03 00:12:31]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=unittests tbd
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests
==========

[DaleCurtis, 2017-05-03 00:12:37]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:13:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-03 00:18:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 00:18:59]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2017-05-03 00:29:08]

Patchset #6 (id:140001) has been deleted

[DaleCurtis, 2017-05-03 00:33:56]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-03 00:34:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-05-03 00:59:34]

dalecurtis@chromium.org changed reviewers:
+ dcheng@chromium.org

[DaleCurtis, 2017-05-03 00:59:34]

Okay this is ready for full review now. All discussions addressed and tests
added.

+dcheng for:
base/BUILD.gn
components/metrics/public/interfaces/single_sample_metrics.mojom
content/public/app/mojo/content_browser_manifest.json

asvitkine@ for base/metrics and components/metrics

nick@ for content/

[commit-bot: I haz the power, 2017-05-03 01:49:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-03 01:49:08]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[DaleCurtis, 2017-05-03 19:40:30]

rockot@ looks like mojo doesn't work on iOS; is that expected?

Assertion failed: (g_thunks.CreateMessagePipe), function MojoCreateMessagePipe,
file ../../mojo/public/c/system/thunks.cc, line 35.
Received signal 6

I'll just exclude this target on iOS if that's the case.

[Ken Rockot(use gerrit already), 2017-05-03 19:50:28]

On 2017/05/03 at 19:40:30, dalecurtis wrote:
> rockot@ looks like mojo doesn't work on iOS; is that expected?
> 
> Assertion failed: (g_thunks.CreateMessagePipe), function
MojoCreateMessagePipe, file ../../mojo/public/c/system/thunks.cc, line 35.
> Received signal 6
> 
> I'll just exclude this target on iOS if that's the case.

That just means Mojo isn't initialized. It's certainly supported on iOS.

What test binary is that from?

[DaleCurtis, 2017-05-03 19:52:37]

On 2017/05/03 at 19:50:28, rockot wrote:
> On 2017/05/03 at 19:40:30, dalecurtis wrote:
> > rockot@ looks like mojo doesn't work on iOS; is that expected?
> > 
> > Assertion failed: (g_thunks.CreateMessagePipe), function
MojoCreateMessagePipe, file ../../mojo/public/c/system/thunks.cc, line 35.
> > Received signal 6
> > 
> > I'll just exclude this target on iOS if that's the case.
> 
> That just means Mojo isn't initialized. It's certainly supported on iOS.
> 
> What test binary is that from?

Okay, probably we're just missing some DEP for this target then. I'll see if I
can track down what's missing.

https://cs.chromium.org/chromium/src/components/metrics/BUILD.gn?l=383

[Ken Rockot(use gerrit already), 2017-05-03 19:54:56]

It's not a dependency issue. It's an issue with mojo::edk::Init() not
having been called in the test process.

Typically this is done in main() before the base TestSuite is launched.
Looks like components/test/run_all_unittests.cc doesn't do this, for
example. That probably explains why it's broken, though that means it's
broken on all platforms and not just iOS.

On Wed, May 3, 2017 at 12:52 PM, <dalecurtis@chromium.org> wrote:

> On 2017/05/03 at 19:50:28, rockot wrote:
> > On 2017/05/03 at 19:40:30, dalecurtis wrote:
> > > rockot@ looks like mojo doesn't work on iOS; is that expected?
> > >
> > > Assertion failed: (g_thunks.CreateMessagePipe), function
> MojoCreateMessagePipe, file ../../mojo/public/c/system/thunks.cc, line 35.
> > > Received signal 6
> > >
> > > I'll just exclude this target on iOS if that's the case.
> >
> > That just means Mojo isn't initialized. It's certainly supported on iOS.
> >
> > What test binary is that from?
>
> Okay, probably we're just missing some DEP for this target then. I'll see
> if I
> can track down what's missing.
>
> https://cs.chromium.org/chromium/src/components/metrics/BUILD.gn?l=383
>
>
>
> https://codereview.chromium.org/2687583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Rockot(use gerrit already), 2017-05-03 19:56:37]

See for example
https://cs.chromium.org/chromium/src/url/run_all_unittests.cc

At one point we didn't support Mojo on iOS which is why a lot of these uses
block it out when defined(OS_IOS), but that's no longer necessary.

On Wed, May 3, 2017 at 12:54 PM, Ken Rockot <rockot@chromium.org> wrote:

> It's not a dependency issue. It's an issue with mojo::edk::Init() not
> having been called in the test process.
>
> Typically this is done in main() before the base TestSuite is launched.
> Looks like components/test/run_all_unittests.cc doesn't do this, for
> example. That probably explains why it's broken, though that means it's
> broken on all platforms and not just iOS.
>
> On Wed, May 3, 2017 at 12:52 PM, <dalecurtis@chromium.org> wrote:
>
>> On 2017/05/03 at 19:50:28, rockot wrote:
>> > On 2017/05/03 at 19:40:30, dalecurtis wrote:
>> > > rockot@ looks like mojo doesn't work on iOS; is that expected?
>> > >
>> > > Assertion failed: (g_thunks.CreateMessagePipe), function
>> MojoCreateMessagePipe, file ../../mojo/public/c/system/thunks.cc, line
>> 35.
>> > > Received signal 6
>> > >
>> > > I'll just exclude this target on iOS if that's the case.
>> >
>> > That just means Mojo isn't initialized. It's certainly supported on iOS.
>> >
>> > What test binary is that from?
>>
>> Okay, probably we're just missing some DEP for this target then. I'll see
>> if I
>> can track down what's missing.
>>
>> https://cs.chromium.org/chromium/src/components/metrics/BUILD.gn?l=383
>>
>>
>>
>> https://codereview.chromium.org/2687583002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2017-05-03 19:57:51]

On 2017/05/03 at 19:56:37, rockot wrote:
> See for example
> https://cs.chromium.org/chromium/src/url/run_all_unittests.cc
> 
> At one point we didn't support Mojo on iOS which is why a lot of these uses
> block it out when defined(OS_IOS), but that's no longer necessary.

Thanks for the pointer. I'm building that target locally (which only runs on the
iOS bots AFAICT), so I'll see if adding the init call is sufficient.

[Alexei Svitkine (slow), 2017-05-03 19:59:46]

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
File base/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:33: // |name| is the name of the histogram.
See base/metrics/histogram_macros.h for
Nit: I'd actually just name the param |histogram_name| and remove this sentence
from this paragraph.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:46: // The factory normally persist until
process shutdown, but in testing we
Nit: persist -> persists

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:50: // The methods below return a single
sample metric for counts histograms.
Nit: Mention corresponding macro this is a parralel to.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:58: // Default implementation for when no
factory has been provided to the process.
Nit: Expand comment to mention data is logged locally by this instance.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:65: // SingleSampleMetricsFactory
implementation.
Nit: Prefer "// SingleSampleMetricsFactory:" syntax for these comments for new
code. Here and below.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:78: DefaultSingleSampleMetric(const
std::string& name,
Nit: histogram_name

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:16: class
SingleSampleMetricsFactoryImpl : public base::SingleSampleMetricsFactory {
Add a comment explaining what this is for.

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:22:
base::RepeatingCallback<void(mojom::SingleSampleMetricsProviderRequest)>;
Nit: Add a new line after this.

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:28: const std::string&
name,
histogram_name

[DaleCurtis, 2017-05-03 20:54:46]

dalecurtis@chromium.org changed reviewers:
+ blundell@chromium.org

[DaleCurtis, 2017-05-03 20:54:48]

iOS hopefully fixed, +blundell for components/test changes to make iOS happy.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
File base/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:33: // |name| is the name of the histogram.
See base/metrics/histogram_macros.h for
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: I'd actually just name the param |histogram_name| and remove this
sentence from this paragraph.

Done.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:46: // The factory normally persist until
process shutdown, but in testing we
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: persist -> persists

Done.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:50: // The methods below return a single
sample metric for counts histograms.
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: Mention corresponding macro this is a parralel to.

Done.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:58: // Default implementation for when no
factory has been provided to the process.
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: Expand comment to mention data is logged locally by this instance.

Done.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:65: // SingleSampleMetricsFactory
implementation.
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: Prefer "// SingleSampleMetricsFactory:" syntax for these comments for new
code. Here and below.

Done.

https://codereview.chromium.org/2687583002/diff/160001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:78: DefaultSingleSampleMetric(const
std::string& name,
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: histogram_name

Done.

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:16: class
SingleSampleMetricsFactoryImpl : public base::SingleSampleMetricsFactory {
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Add a comment explaining what this is for.

Done.

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:22:
base::RepeatingCallback<void(mojom::SingleSampleMetricsProviderRequest)>;
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> Nit: Add a new line after this.

Rearranged.

https://codereview.chromium.org/2687583002/diff/160001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:28: const std::string&
name,
On 2017/05/03 at 19:59:46, Alexei Svitkine (slow) wrote:
> histogram_name

Done.

[Alexei Svitkine (slow), 2017-05-03 21:18:48]

https://codereview.chromium.org/2687583002/diff/180001/base/metrics/single_sa...
File base/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/180001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:43: static void
SetFactory(SingleSampleMetricsFactory* factory);
Pass by unique_ptr to indicate ownership transfer.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
Hmm, I find this naming unfortunate - because we already have a notion of
"metrics provider" that is something else - and I'd rather not add confusion by
naming this also a "metrics provider".

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
File components/metrics/public/interfaces/single_sample_metrics.mojom (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
components/metrics/public/interfaces/single_sample_metrics.mojom:7: interface
SingleSampleMetricsProvider {
Add a comment mentioning to see single_sample_metrics_factory_impl.h for
details.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
components/metrics/public/interfaces/single_sample_metrics.mojom:16:
AcquireSingleSampleMetric(string name, int32 min, int32 max,
Nit: histogram_name

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.cc:11: class
SingleSampleMetricImpl : public base::SingleSampleMetric {
Can this be in the anon namespace?

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.cc:75:
provider_.Set(provider);
That's a lot of indirection. Might be good to call out this set up in a comment
somewhere. i.e. explaining the three objects (tls pointer to mojo pointer to
actual object)

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
Can you mention somewhere in the comments the purpose of the callback?

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
Hmm, it seems strange that this is constructing an Impl object from the metrics
component. Impl sounds like it's implementation detail that shouldn't be exposed
outside the component.

How about keeping _impl.h private to the component and having a helper function
in metrics component that returns the instance?

[DaleCurtis, 2017-05-03 21:37:52]

Most comments addressed, need more information on some asvitkine@.

https://codereview.chromium.org/2687583002/diff/180001/base/metrics/single_sa...
File base/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/180001/base/metrics/single_sa...
base/metrics/single_sample_metrics.h:43: static void
SetFactory(SingleSampleMetricsFactory* factory);
On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> Pass by unique_ptr to indicate ownership transfer.

Done.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> Hmm, I find this naming unfortunate - because we already have a notion of
"metrics provider" that is something else - and I'd rather not add confusion by
naming this also a "metrics provider".

This is naming convention used by mojo for these type of objects. So we're in
trouble either way it sounds like. I.e. break convention in mojo or break
convention in metrics. I don't have a strong opinion either way, so up to you.
Do you have a naming suggestion?

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
File components/metrics/public/interfaces/single_sample_metrics.mojom (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
components/metrics/public/interfaces/single_sample_metrics.mojom:7: interface
SingleSampleMetricsProvider {
On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> Add a comment mentioning to see single_sample_metrics_factory_impl.h for
details.

Done.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/pub...
components/metrics/public/interfaces/single_sample_metrics.mojom:16:
AcquireSingleSampleMetric(string name, int32 min, int32 max,
On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> Nit: histogram_name

Done.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.cc:11: class
SingleSampleMetricImpl : public base::SingleSampleMetric {
On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> Can this be in the anon namespace?

Done.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.cc:75:
provider_.Set(provider);
On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> That's a lot of indirection. Might be good to call out this set up in a
comment somewhere. i.e. explaining the three objects (tls pointer to mojo
pointer to actual object)

Comments added.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> Can you mention somewhere in the comments the purpose of the callback?

What's do you think is missing from the current "|create_provider_cb| will be
called..." sentence?

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> Hmm, it seems strange that this is constructing an Impl object from the
metrics component. Impl sounds like it's implementation detail that shouldn't be
exposed outside the component.
> 
> How about keeping _impl.h private to the component and having a helper
function in metrics component that returns the instance?

You mean something like a "static
metrics::InitializeSingleSampleMetrics(callback)" ?

[Alexei Svitkine (slow), 2017-05-03 21:45:08]

Thanks!

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
On 2017/05/03 21:37:51, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> > Hmm, I find this naming unfortunate - because we already have a notion of
> "metrics provider" that is something else - and I'd rather not add confusion
by
> naming this also a "metrics provider".
> 
> This is naming convention used by mojo for these type of objects. So we're in
> trouble either way it sounds like. I.e. break convention in mojo or break
> convention in metrics. I don't have a strong opinion either way, so up to you.
> Do you have a naming suggestion?

Perhaps we can put something between "metrics" and "provider" so it's not using
those words together, at least?

e.g. "single_sample_metrics_mojo_provider.cc"?

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > Can you mention somewhere in the comments the purpose of the callback?
> 
> What's do you think is missing from the current "|create_provider_cb| will be
> called..." sentence?

I meant to explain why it's necessary for it be provided externally - e.g.
what's the use case for passing different callbacks to this?

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > Hmm, it seems strange that this is constructing an Impl object from the
> metrics component. Impl sounds like it's implementation detail that shouldn't
be
> exposed outside the component.
> > 
> > How about keeping _impl.h private to the component and having a helper
> function in metrics component that returns the instance?
> 
> You mean something like a "static
> metrics::InitializeSingleSampleMetrics(callback)" ?

Yep!

[DaleCurtis, 2017-05-04 02:29:00]

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> On 2017/05/03 21:37:51, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> > > Hmm, I find this naming unfortunate - because we already have a notion of
> > "metrics provider" that is something else - and I'd rather not add confusion
by
> > naming this also a "metrics provider".
> > 
> > This is naming convention used by mojo for these type of objects. So we're
in
> > trouble either way it sounds like. I.e. break convention in mojo or break
> > convention in metrics. I don't have a strong opinion either way, so up to
you.
> > Do you have a naming suggestion?
> 
> Perhaps we can put something between "metrics" and "provider" so it's not
using those words together, at least?
> 
> e.g. "single_sample_metrics_mojo_provider.cc"?

Okay, I think I have a compromise which works. I've removed this source file and
deduped the tests by making the implementation of
mojom::SingleSampleMetricsProvider an internal detail of the static helpers. We
still have a mojo class and a private class which end in MetricsProvider, but
they are not exposed elsewhere. WDYT?

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > Can you mention somewhere in the comments the purpose of the callback?
> > 
> > What's do you think is missing from the current "|create_provider_cb| will
be
> > called..." sentence?
> 
> I meant to explain why it's necessary for it be provided externally - e.g.
what's the use case for passing different callbacks to this?

I'm not sure what you mean? Probably clients will always pass the same callback
for construction.

As for why a callback? We pass a callback here so this code doesn't have to take
DEPS on content for service names or require a service_manager::Connector() to
construct for testing (which is a huge pain since that's a whole custom test
harness).

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > Hmm, it seems strange that this is constructing an Impl object from the
> > metrics component. Impl sounds like it's implementation detail that
shouldn't be
> > exposed outside the component.
> > > 
> > > How about keeping _impl.h private to the component and having a helper
> > function in metrics component that returns the instance?
> > 
> > You mean something like a "static
> > metrics::InitializeSingleSampleMetrics(callback)" ?
> 
> Yep!

Done.

[DaleCurtis, 2017-05-04 02:29:08]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 02:29:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 02:41:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 02:41:23]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2017-05-04 03:09:25]

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
On 2017/05/04 at 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > On 2017/05/03 21:37:51, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> > > > Hmm, I find this naming unfortunate - because we already have a notion
of
> > > "metrics provider" that is something else - and I'd rather not add
confusion by
> > > naming this also a "metrics provider".
> > > 
> > > This is naming convention used by mojo for these type of objects. So we're
in
> > > trouble either way it sounds like. I.e. break convention in mojo or break
> > > convention in metrics. I don't have a strong opinion either way, so up to
you.
> > > Do you have a naming suggestion?
> > 
> > Perhaps we can put something between "metrics" and "provider" so it's not
using those words together, at least?
> > 
> > e.g. "single_sample_metrics_mojo_provider.cc"?
> 
> Okay, I think I have a compromise which works. I've removed this source file
and deduped the tests by making the implementation of
mojom::SingleSampleMetricsProvider an internal detail of the static helpers. We
still have a mojo class and a private class which end in MetricsProvider, but
they are not exposed elsewhere. WDYT?

Ah, actually this doesn't work since AddInterface() needs to infer the
implementation name from the ::Create method. So we must expose the class
somehow. I'll redo this tomorrow.

[DaleCurtis, 2017-05-04 03:09:34]

Patchset #9 (id:220001) has been deleted

[Alexei Svitkine (slow), 2017-05-04 14:15:09]

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
On 2017/05/04 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > > Can you mention somewhere in the comments the purpose of the callback?
> > > 
> > > What's do you think is missing from the current "|create_provider_cb| will
> be
> > > called..." sentence?
> > 
> > I meant to explain why it's necessary for it be provided externally - e.g.
> what's the use case for passing different callbacks to this?
> 
> I'm not sure what you mean? Probably clients will always pass the same
callback
> for construction.
> 
> As for why a callback? We pass a callback here so this code doesn't have to
take
> DEPS on content for service names or require a service_manager::Connector() to
> construct for testing (which is a huge pain since that's a whole custom test
> harness).

Sorry for the confusion - I meant that your explanation above (about DEPS /
service names) could be added to the comment. That part explains why we're using
a callback here.

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
On 2017/05/04 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > > Hmm, it seems strange that this is constructing an Impl object from the
> > > metrics component. Impl sounds like it's implementation detail that
> shouldn't be
> > > exposed outside the component.
> > > > 
> > > > How about keeping _impl.h private to the component and having a helper
> > > function in metrics component that returns the instance?
> > > 
> > > You mean something like a "static
> > > metrics::InitializeSingleSampleMetrics(callback)" ?
> > 
> > Yep!
> 
> Done.

Hmm, did you forget to upload a patchset? Don't see this part of the change...

[blundell, 2017-05-04 14:58:18]

//components/test lgtm

[DaleCurtis, 2017-05-04 18:46:18]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 18:46:40]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-05-04 18:55:27]

Okay I think all is resolved now. PTAL, thanks! (FYI, asvitkine@ I'm OOO
starting tomorrow for two weeks, so apologies if I'm not able to submit this
before leaving).

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
File components/metrics/BUILD.gn (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/BUI...
components/metrics/BUILD.gn:258: "single_sample_metrics_provider.cc",
On 2017/05/04 at 03:09:24, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/04 at 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > > On 2017/05/03 21:37:51, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > > On 2017/05/03 at 21:18:47, Alexei Svitkine (slow) wrote:
> > > > > Hmm, I find this naming unfortunate - because we already have a notion
of
> > > > "metrics provider" that is something else - and I'd rather not add
confusion by
> > > > naming this also a "metrics provider".
> > > > 
> > > > This is naming convention used by mojo for these type of objects. So
we're in
> > > > trouble either way it sounds like. I.e. break convention in mojo or
break
> > > > convention in metrics. I don't have a strong opinion either way, so up
to you.
> > > > Do you have a naming suggestion?
> > > 
> > > Perhaps we can put something between "metrics" and "provider" so it's not
using those words together, at least?
> > > 
> > > e.g. "single_sample_metrics_mojo_provider.cc"?
> > 
> > Okay, I think I have a compromise which works. I've removed this source file
and deduped the tests by making the implementation of
mojom::SingleSampleMetricsProvider an internal detail of the static helpers. We
still have a mojo class and a private class which end in MetricsProvider, but
they are not exposed elsewhere. WDYT?
> 
> Ah, actually this doesn't work since AddInterface() needs to infer the
implementation name from the ::Create method. So we must expose the class
somehow. I'll redo this tomorrow.

Oooh, fixed my template errors so this is working now. PTAL.

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
File components/metrics/single_sample_metrics_factory_impl.h (right):

https://codereview.chromium.org/2687583002/diff/180001/components/metrics/sin...
components/metrics/single_sample_metrics_factory_impl.h:31: explicit
SingleSampleMetricsFactoryImpl(CreateProviderCB create_provider_cb);
On 2017/05/04 at 14:15:08, Alexei Svitkine (slow) wrote:
> On 2017/05/04 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > > On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > > > Can you mention somewhere in the comments the purpose of the callback?
> > > > 
> > > > What's do you think is missing from the current "|create_provider_cb|
will
> > be
> > > > called..." sentence?
> > > 
> > > I meant to explain why it's necessary for it be provided externally - e.g.
> > what's the use case for passing different callbacks to this?
> > 
> > I'm not sure what you mean? Probably clients will always pass the same
callback
> > for construction.
> > 
> > As for why a callback? We pass a callback here so this code doesn't have to
take
> > DEPS on content for service names or require a service_manager::Connector()
to
> > construct for testing (which is a huge pain since that's a whole custom test
> > harness).
> 
> Sorry for the confusion - I meant that your explanation above (about DEPS /
service names) could be added to the comment. That part explains why we're using
a callback here.

Ah I see, done.

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/180001/content/renderer/rende...
content/renderer/render_thread_impl.cc:638: new
metrics::SingleSampleMetricsFactoryImpl(base::BindRepeating(
On 2017/05/04 at 14:15:08, Alexei Svitkine (slow) wrote:
> On 2017/05/04 02:28:59, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/03 at 21:45:08, Alexei Svitkine (slow) wrote:
> > > On 2017/05/03 21:37:52, DaleCurtis_OOO_May_5_To_May23 wrote:
> > > > On 2017/05/03 at 21:18:48, Alexei Svitkine (slow) wrote:
> > > > > Hmm, it seems strange that this is constructing an Impl object from
the
> > > > metrics component. Impl sounds like it's implementation detail that
> > shouldn't be
> > > > exposed outside the component.
> > > > > 
> > > > > How about keeping _impl.h private to the component and having a helper
> > > > function in metrics component that returns the instance?
> > > > 
> > > > You mean something like a "static
> > > > metrics::InitializeSingleSampleMetrics(callback)" ?
> > > 
> > > Yep!
> > 
> > Done.
> 
> Hmm, did you forget to upload a patchset? Don't see this part of the change...

No intentionally didn't upload since it was busted but forgot I posted this.
Fixed in latest patch set.

[DaleCurtis, 2017-05-04 18:58:50]

The CQ bit was unchecked by dalecurtis@chromium.org

[Alexei Svitkine (slow), 2017-05-04 19:03:22]

LGTM - thanks!

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:78: const
service_manager::BindSourceInfo& /* source_info */,
Nit: I don't think most of Chrome follows the convention of commenting out
unused params. Certainly we don't in metrics component - so just specify the
param normally.

[DaleCurtis, 2017-05-04 19:08:50]

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:78: const
service_manager::BindSourceInfo& /* source_info */,
On 2017/05/04 at 19:03:21, Alexei Svitkine (slow) wrote:
> Nit: I don't think most of Chrome follows the convention of commenting out
unused params. Certainly we don't in metrics component - so just specify the
param normally.

I can change it, but this is codified in the style guide and we do use it in
media and blink at least:

https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...

Happy to change if you prefer otherwise.

[Alexei Svitkine (slow), 2017-05-04 19:14:20]

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:78: const
service_manager::BindSourceInfo& /* source_info */,
On 2017/05/04 19:08:50, DaleCurtis_OOO_May_5_To_May23 wrote:
> On 2017/05/04 at 19:03:21, Alexei Svitkine (slow) wrote:
> > Nit: I don't think most of Chrome follows the convention of commenting out
> unused params. Certainly we don't in metrics component - so just specify the
> param normally.
> 
> I can change it, but this is codified in the style guide and we do use it in
> media and blink at least:
> 
>
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
> 
> Happy to change if you prefer otherwise.

This has been discussed previously on chromium-dev@ here:

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TBaVejz_hGQ/5eHIZ...

My understanding is there was a preference for not doing this and that
commenting them out is allowed by style guide simply because some projects
compile themselves with settings where it would otherwise cause a warning. But
Chromium doesn't, so we shouldn't have the extra noise from it in the codebase.

[DaleCurtis, 2017-05-04 21:03:39]

dcheng@, nick@ PTAL, thanks!

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/260001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:78: const
service_manager::BindSourceInfo& /* source_info */,
On 2017/05/04 at 19:14:20, Alexei Svitkine (slow) wrote:
> On 2017/05/04 19:08:50, DaleCurtis_OOO_May_5_To_May23 wrote:
> > On 2017/05/04 at 19:03:21, Alexei Svitkine (slow) wrote:
> > > Nit: I don't think most of Chrome follows the convention of commenting out
> > unused params. Certainly we don't in metrics component - so just specify the
> > param normally.
> > 
> > I can change it, but this is codified in the style guide and we do use it in
> > media and blink at least:
> > 
> >
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_D...
> > 
> > Happy to change if you prefer otherwise.
> 
> This has been discussed previously on chromium-dev@ here:
> 
>
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/TBaVejz_hGQ/5eHIZ...
> 
> My understanding is there was a preference for not doing this and that
commenting them out is allowed by style guide simply because some projects
compile themselves with settings where it would otherwise cause a warning. But
Chromium doesn't, so we shouldn't have the extra noise from it in the codebase.

Okay, done! Thanks for the link.

[DaleCurtis, 2017-05-04 21:03:58]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-04 21:04:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ncarter (slow), 2017-05-04 21:26:14]

https://codereview.chromium.org/2687583002/diff/300001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/300001/content/renderer/rende...
content/renderer/render_thread_impl.cc:398: // Necessary since BindInterface()
is a template and can't be bound directly.
This comment seems inaccurate, since the PostTask behavior also necessitates a
helper function. I'd rather just see a comment that describes the role this
function plays:

// Hook that allows single-sample metric code from //components/metrics
// to connect from the renderer process to the browser process.

[ncarter (slow), 2017-05-04 21:26:44]

lgtm modulo the previous comment about comments

[DaleCurtis, 2017-05-04 21:31:08]

https://codereview.chromium.org/2687583002/diff/300001/content/renderer/rende...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/2687583002/diff/300001/content/renderer/rende...
content/renderer/render_thread_impl.cc:398: // Necessary since BindInterface()
is a template and can't be bound directly.
On 2017/05/04 at 21:26:14, ncarter wrote:
> This comment seems inaccurate, since the PostTask behavior also necessitates a
helper function. I'd rather just see a comment that describes the role this
function plays:
> 
> // Hook that allows single-sample metric code from //components/metrics
> // to connect from the renderer process to the browser process.

Done.

[dcheng, 2017-05-04 22:18:51]

base and mojom LGTM to unblock things

That being said, it feels like this has a fair amount of overlap with the shmem
UMA infrastructure: IIRC, we developed the shmem UMA to support use counters
(which are tied to page views) properly even on fast renderer shutdown.

I'm not actually sure how the shmem UMA works though; bcwhite@, do you have
links to designs, documentation, etc?

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:73: std::move(create_provider_cb)));
Nit: #include <utility>

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
File components/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
components/metrics/single_sample_metrics.h:23: // service_manager::Connector()
for simplicitly and to avoid the need for
simplicitly => simplicity

[DaleCurtis, 2017-05-04 22:25:03]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-05-04 22:25:05]

Thanks for review everyone!

dcheng@, I believe this points to the persistent histogram design docs:

https://bugs.chromium.org/p/chromium/issues/detail?id=583440

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
File components/metrics/single_sample_metrics.cc (right):

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
components/metrics/single_sample_metrics.cc:73: std::move(create_provider_cb)));
On 2017/05/04 at 22:18:51, dcheng wrote:
> Nit: #include <utility>

Done.

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
File components/metrics/single_sample_metrics.h (right):

https://codereview.chromium.org/2687583002/diff/340001/components/metrics/sin...
components/metrics/single_sample_metrics.h:23: // service_manager::Connector()
for simplicitly and to avoid the need for
On 2017/05/04 at 22:18:51, dcheng wrote:
> simplicitly => simplicity

Done.

[DaleCurtis, 2017-05-04 22:25:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
blundell@chromium.org, asvitkine@chromium.org, nick@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2687583002/#ps360001
(title: "Address comments.")

[commit-bot: I haz the power, 2017-05-04 22:25:53]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 22:39:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-04 22:39:15]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[DaleCurtis, 2017-05-04 22:40:29]

rockot@ can you approve for mojo/edk DEP in component/test

[Ken Rockot(use gerrit already), 2017-05-04 22:41:48]

lgtm

[DaleCurtis, 2017-05-04 22:41:59]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-05-04 22:42:00]

Thanks!

[commit-bot: I haz the power, 2017-05-04 22:42:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-04 23:40:50]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1493937719339480,
"parent_rev": "b59f96b265d0bfb15ee56647ab4e3e28ae886b38", "commit_rev":
"4a9839a21d7fd95641ba584396bf328fbc1a96b0"}

[commit-bot: I haz the power, 2017-05-04 23:41:03]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests

Review-Url: https://codereview.chromium.org/2687583002
Cr-Commit-Position: refs/heads/master@{#469524}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
==========

[commit-bot: I haz the power, 2017-05-04 23:41:05]

Committed patchset #15 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...

[xunjieli, 2017-05-05 00:33:41]

This CL drastically increased Cronet binary size (~40% increase for our debug
build). Cronet depends on components/metrics.
Perf alerts are at https://chromeperf.appspot.com/group_report?rev=469524.
Can we revert this CL and evaluate how we can avoid the added dependency?

[xunjieli, 2017-05-05 00:34:15]

Description was changed from

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests

Review-Url: https://codereview.chromium.org/2687583002
Cr-Commit-Position: refs/heads/master@{#469524}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
==========

to

==========
Add support for single sample metrics.

Single sample metrics offer a mechanism for clients to modify
a histogram sample repeatedly while having it only reported once;
either upon destruction of a helper class or at process termination.

This is helpful since the fast shutdown path simply kills renderer
processes without going through any destructors. In this case we
lose histogram values that might otherwise be reported at that
time.

In media/ code we've created a cumbersome proxy mechanism which
sends histogram values like this to the browser process and records
them there (see WatchTimeReporter and MediaInternals interactions).

Single sample histograms are implemented through a new mojo service
hosted by the browser process that receives samples and logs the
last received sample upon mojo channel closure.

base:: shims are provided so that these metrics can be created by
anyone in the renderer process. For non-renderer processes a default
implementation is provided without the mojo channel behavior.

Usage looks like the following:
  std::unique_ptr<base::SingleSampleMetric> metric(
      base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
          "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
  metric->SetSample(1);
  metric->SetSample(2);
  metric->SetSample(3);
  metric.reset();

Only the last sample (3) would end up being reported to the histogram.

BUG=689751
TEST=new tests

Review-Url: https://codereview.chromium.org/2687583002
Cr-Commit-Position: refs/heads/master@{#469524}
Committed:
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
==========

[dcheng, 2017-05-05 04:57:53]

On 2017/05/05 00:34:15, xunjieli wrote:
> Description was changed from
> 
> ==========
> Add support for single sample metrics.
> 
> Single sample metrics offer a mechanism for clients to modify
> a histogram sample repeatedly while having it only reported once;
> either upon destruction of a helper class or at process termination.
> 
> This is helpful since the fast shutdown path simply kills renderer
> processes without going through any destructors. In this case we
> lose histogram values that might otherwise be reported at that
> time.
> 
> In media/ code we've created a cumbersome proxy mechanism which
> sends histogram values like this to the browser process and records
> them there (see WatchTimeReporter and MediaInternals interactions).
> 
> Single sample histograms are implemented through a new mojo service
> hosted by the browser process that receives samples and logs the
> last received sample upon mojo channel closure.
> 
> base:: shims are provided so that these metrics can be created by
> anyone in the renderer process. For non-renderer processes a default
> implementation is provided without the mojo channel behavior.
> 
> Usage looks like the following:
>   std::unique_ptr<base::SingleSampleMetric> metric(
>       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
>           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
>   metric->SetSample(1);
>   metric->SetSample(2);
>   metric->SetSample(3);
>   metric.reset();
> 
> Only the last sample (3) would end up being reported to the histogram.
> 
> BUG=689751
> TEST=new tests
> 
> Review-Url: https://codereview.chromium.org/2687583002
> Cr-Commit-Position: refs/heads/master@{#469524}
> Committed:
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> ==========
> 
> to
> 
> ==========
> Add support for single sample metrics.
> 
> Single sample metrics offer a mechanism for clients to modify
> a histogram sample repeatedly while having it only reported once;
> either upon destruction of a helper class or at process termination.
> 
> This is helpful since the fast shutdown path simply kills renderer
> processes without going through any destructors. In this case we
> lose histogram values that might otherwise be reported at that
> time.
> 
> In media/ code we've created a cumbersome proxy mechanism which
> sends histogram values like this to the browser process and records
> them there (see WatchTimeReporter and MediaInternals interactions).
> 
> Single sample histograms are implemented through a new mojo service
> hosted by the browser process that receives samples and logs the
> last received sample upon mojo channel closure.
> 
> base:: shims are provided so that these metrics can be created by
> anyone in the renderer process. For non-renderer processes a default
> implementation is provided without the mojo channel behavior.
> 
> Usage looks like the following:
>   std::unique_ptr<base::SingleSampleMetric> metric(
>       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
>           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
>   metric->SetSample(1);
>   metric->SetSample(2);
>   metric->SetSample(3);
>   metric.reset();
> 
> Only the last sample (3) would end up being reported to the histogram.
> 
> BUG=689751
> TEST=new tests
> 
> Review-Url: https://codereview.chromium.org/2687583002
> Cr-Commit-Position: refs/heads/master@{#469524}
> Committed:
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> ==========

I thought about this more, and I think we should probably try to implement this
without Mojo. Even though Mojo is pretty lightweight, having one message pipe
per process recording these UMAs doesn't seem a bit heavy. Assuming the shmem
UMAs aren't sharing segments between renderers, we can just record the UMA data
in the shared memory segment, with an additional int32 of storage for
single-sample histograms: this holds the 'pending' value. This pending value can
be explicitly flushed via destructor invocation, or it can be implicitly flushed
when the renderer process exits.

[lijeffrey1, 2017-05-05 18:13:57]

On 2017/05/05 04:57:53, dcheng wrote:
> On 2017/05/05 00:34:15, xunjieli wrote:
> > Description was changed from
> > 
> > ==========
> > Add support for single sample metrics.
> > 
> > Single sample metrics offer a mechanism for clients to modify
> > a histogram sample repeatedly while having it only reported once;
> > either upon destruction of a helper class or at process termination.
> > 
> > This is helpful since the fast shutdown path simply kills renderer
> > processes without going through any destructors. In this case we
> > lose histogram values that might otherwise be reported at that
> > time.
> > 
> > In media/ code we've created a cumbersome proxy mechanism which
> > sends histogram values like this to the browser process and records
> > them there (see WatchTimeReporter and MediaInternals interactions).
> > 
> > Single sample histograms are implemented through a new mojo service
> > hosted by the browser process that receives samples and logs the
> > last received sample upon mojo channel closure.
> > 
> > base:: shims are provided so that these metrics can be created by
> > anyone in the renderer process. For non-renderer processes a default
> > implementation is provided without the mojo channel behavior.
> > 
> > Usage looks like the following:
> >   std::unique_ptr<base::SingleSampleMetric> metric(
> >       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> >           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> >   metric->SetSample(1);
> >   metric->SetSample(2);
> >   metric->SetSample(3);
> >   metric.reset();
> > 
> > Only the last sample (3) would end up being reported to the histogram.
> > 
> > BUG=689751
> > TEST=new tests
> > 
> > Review-Url: https://codereview.chromium.org/2687583002
> > Cr-Commit-Position: refs/heads/master@{#469524}
> > Committed:
> >
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> > ==========
> > 
> > to
> > 
> > ==========
> > Add support for single sample metrics.
> > 
> > Single sample metrics offer a mechanism for clients to modify
> > a histogram sample repeatedly while having it only reported once;
> > either upon destruction of a helper class or at process termination.
> > 
> > This is helpful since the fast shutdown path simply kills renderer
> > processes without going through any destructors. In this case we
> > lose histogram values that might otherwise be reported at that
> > time.
> > 
> > In media/ code we've created a cumbersome proxy mechanism which
> > sends histogram values like this to the browser process and records
> > them there (see WatchTimeReporter and MediaInternals interactions).
> > 
> > Single sample histograms are implemented through a new mojo service
> > hosted by the browser process that receives samples and logs the
> > last received sample upon mojo channel closure.
> > 
> > base:: shims are provided so that these metrics can be created by
> > anyone in the renderer process. For non-renderer processes a default
> > implementation is provided without the mojo channel behavior.
> > 
> > Usage looks like the following:
> >   std::unique_ptr<base::SingleSampleMetric> metric(
> >       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> >           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> >   metric->SetSample(1);
> >   metric->SetSample(2);
> >   metric->SetSample(3);
> >   metric.reset();
> > 
> > Only the last sample (3) would end up being reported to the histogram.
> > 
> > BUG=689751
> > TEST=new tests
> > 
> > Review-Url: https://codereview.chromium.org/2687583002
> > Cr-Commit-Position: refs/heads/master@{#469524}
> > Committed:
> >
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> > ==========
> 
> I thought about this more, and I think we should probably try to implement
this
> without Mojo. Even though Mojo is pretty lightweight, having one message pipe
> per process recording these UMAs doesn't seem a bit heavy. Assuming the shmem
> UMAs aren't sharing segments between renderers, we can just record the UMA
data
> in the shared memory segment, with an additional int32 of storage for
> single-sample histograms: this holds the 'pending' value. This pending value
can
> be explicitly flushed via destructor invocation, or it can be implicitly
flushed
> when the renderer process exits.

Findit suggests this CL added a flaky test
"SingleSampleMetricsFactoryImplTest.MultithreadedMetrics" according to analysis
https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV...

Can someone please help confirm?

Thanks,
Jeff on behalf of Findit team

[dcheng, 2017-05-05 18:18:18]

On 2017/05/05 18:13:57, lijeffrey1 wrote:
> On 2017/05/05 04:57:53, dcheng wrote:
> > On 2017/05/05 00:34:15, xunjieli wrote:
> > > Description was changed from
> > > 
> > > ==========
> > > Add support for single sample metrics.
> > > 
> > > Single sample metrics offer a mechanism for clients to modify
> > > a histogram sample repeatedly while having it only reported once;
> > > either upon destruction of a helper class or at process termination.
> > > 
> > > This is helpful since the fast shutdown path simply kills renderer
> > > processes without going through any destructors. In this case we
> > > lose histogram values that might otherwise be reported at that
> > > time.
> > > 
> > > In media/ code we've created a cumbersome proxy mechanism which
> > > sends histogram values like this to the browser process and records
> > > them there (see WatchTimeReporter and MediaInternals interactions).
> > > 
> > > Single sample histograms are implemented through a new mojo service
> > > hosted by the browser process that receives samples and logs the
> > > last received sample upon mojo channel closure.
> > > 
> > > base:: shims are provided so that these metrics can be created by
> > > anyone in the renderer process. For non-renderer processes a default
> > > implementation is provided without the mojo channel behavior.
> > > 
> > > Usage looks like the following:
> > >   std::unique_ptr<base::SingleSampleMetric> metric(
> > >       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> > >           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> > >   metric->SetSample(1);
> > >   metric->SetSample(2);
> > >   metric->SetSample(3);
> > >   metric.reset();
> > > 
> > > Only the last sample (3) would end up being reported to the histogram.
> > > 
> > > BUG=689751
> > > TEST=new tests
> > > 
> > > Review-Url: https://codereview.chromium.org/2687583002
> > > Cr-Commit-Position: refs/heads/master@{#469524}
> > > Committed:
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> > > ==========
> > > 
> > > to
> > > 
> > > ==========
> > > Add support for single sample metrics.
> > > 
> > > Single sample metrics offer a mechanism for clients to modify
> > > a histogram sample repeatedly while having it only reported once;
> > > either upon destruction of a helper class or at process termination.
> > > 
> > > This is helpful since the fast shutdown path simply kills renderer
> > > processes without going through any destructors. In this case we
> > > lose histogram values that might otherwise be reported at that
> > > time.
> > > 
> > > In media/ code we've created a cumbersome proxy mechanism which
> > > sends histogram values like this to the browser process and records
> > > them there (see WatchTimeReporter and MediaInternals interactions).
> > > 
> > > Single sample histograms are implemented through a new mojo service
> > > hosted by the browser process that receives samples and logs the
> > > last received sample upon mojo channel closure.
> > > 
> > > base:: shims are provided so that these metrics can be created by
> > > anyone in the renderer process. For non-renderer processes a default
> > > implementation is provided without the mojo channel behavior.
> > > 
> > > Usage looks like the following:
> > >   std::unique_ptr<base::SingleSampleMetric> metric(
> > >       base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> > >           "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> > >   metric->SetSample(1);
> > >   metric->SetSample(2);
> > >   metric->SetSample(3);
> > >   metric.reset();
> > > 
> > > Only the last sample (3) would end up being reported to the histogram.
> > > 
> > > BUG=689751
> > > TEST=new tests
> > > 
> > > Review-Url: https://codereview.chromium.org/2687583002
> > > Cr-Commit-Position: refs/heads/master@{#469524}
> > > Committed:
> > >
> >
>
https://chromium.googlesource.com/chromium/src/+/4a9839a21d7fd95641ba584396bf...
> > > ==========
> > 
> > I thought about this more, and I think we should probably try to implement
> this
> > without Mojo. Even though Mojo is pretty lightweight, having one message
pipe
> > per process recording these UMAs doesn't seem a bit heavy. Assuming the
shmem
> > UMAs aren't sharing segments between renderers, we can just record the UMA
> data
> > in the shared memory segment, with an additional int32 of storage for
> > single-sample histograms: this holds the 'pending' value. This pending value
> can
> > be explicitly flushed via destructor invocation, or it can be implicitly
> flushed
> > when the renderer process exits.
> 
> Findit suggests this CL added a flaky test
> "SingleSampleMetricsFactoryImplTest.MultithreadedMetrics" according to
analysis
>
https://findit-for-me.appspot.com/waterfall/flake?key=ag9zfmZpbmRpdC1mb3ItbWV...
> 
> Can someone please help confirm?
> 
> Thanks,
> Jeff on behalf of Findit team

dalecurtis is OOO. Are there features that are blocked on this CL? If not, maybe
we should consider reverting given the size issues with cronet?

[Alexei Svitkine (slow), 2017-05-05 18:22:02]

I don't think anything is blocked on this CL, so reverting seems reasonable.

On Fri, May 5, 2017 at 2:18 PM, <dcheng@chromium.org> wrote:

> On 2017/05/05 18:13:57, lijeffrey1 wrote:
> > On 2017/05/05 04:57:53, dcheng wrote:
> > > On 2017/05/05 00:34:15, xunjieli wrote:
> > > > Description was changed from
> > > >
> > > > ==========
> > > > Add support for single sample metrics.
> > > >
> > > > Single sample metrics offer a mechanism for clients to modify
> > > > a histogram sample repeatedly while having it only reported once;
> > > > either upon destruction of a helper class or at process termination.
> > > >
> > > > This is helpful since the fast shutdown path simply kills renderer
> > > > processes without going through any destructors. In this case we
> > > > lose histogram values that might otherwise be reported at that
> > > > time.
> > > >
> > > > In media/ code we've created a cumbersome proxy mechanism which
> > > > sends histogram values like this to the browser process and records
> > > > them there (see WatchTimeReporter and MediaInternals interactions).
> > > >
> > > > Single sample histograms are implemented through a new mojo service
> > > > hosted by the browser process that receives samples and logs the
> > > > last received sample upon mojo channel closure.
> > > >
> > > > base:: shims are provided so that these metrics can be created by
> > > > anyone in the renderer process. For non-renderer processes a default
> > > > implementation is provided without the mojo channel behavior.
> > > >
> > > > Usage looks like the following:
> > > > std::unique_ptr<base::SingleSampleMetric> metric(
> > > > base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> > > > "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> > > > metric->SetSample(1);
> > > > metric->SetSample(2);
> > > > metric->SetSample(3);
> > > > metric.reset();
> > > >
> > > > Only the last sample (3) would end up being reported to the
> histogram.
> > > >
> > > > BUG=689751
> > > > TEST=new tests
> > > >
> > > > Review-Url: https://codereview.chromium.org/2687583002
> > > > Cr-Commit-Position: refs/heads/master@{#469524}
> > > > Committed:
> > > >
> > >
> >
> https://chromium.googlesource.com/chromium/src/+/
> 4a9839a21d7fd95641ba584396bf328fbc1a96b0
> > > > ==========
> > > >
> > > > to
> > > >
> > > > ==========
> > > > Add support for single sample metrics.
> > > >
> > > > Single sample metrics offer a mechanism for clients to modify
> > > > a histogram sample repeatedly while having it only reported once;
> > > > either upon destruction of a helper class or at process termination.
> > > >
> > > > This is helpful since the fast shutdown path simply kills renderer
> > > > processes without going through any destructors. In this case we
> > > > lose histogram values that might otherwise be reported at that
> > > > time.
> > > >
> > > > In media/ code we've created a cumbersome proxy mechanism which
> > > > sends histogram values like this to the browser process and records
> > > > them there (see WatchTimeReporter and MediaInternals interactions).
> > > >
> > > > Single sample histograms are implemented through a new mojo service
> > > > hosted by the browser process that receives samples and logs the
> > > > last received sample upon mojo channel closure.
> > > >
> > > > base:: shims are provided so that these metrics can be created by
> > > > anyone in the renderer process. For non-renderer processes a default
> > > > implementation is provided without the mojo channel behavior.
> > > >
> > > > Usage looks like the following:
> > > > std::unique_ptr<base::SingleSampleMetric> metric(
> > > > base::SingleSampleMetricsFactory::Get()->CreateCustomCountsMetric(
> > > > "Media.VideoRenderer.CadenceChanges", 1, 10, 10));
> > > > metric->SetSample(1);
> > > > metric->SetSample(2);
> > > > metric->SetSample(3);
> > > > metric.reset();
> > > >
> > > > Only the last sample (3) would end up being reported to the
> histogram.
> > > >
> > > > BUG=689751
> > > > TEST=new tests
> > > >
> > > > Review-Url: https://codereview.chromium.org/2687583002
> > > > Cr-Commit-Position: refs/heads/master@{#469524}
> > > > Committed:
> > > >
> > >
> >
> https://chromium.googlesource.com/chromium/src/+/
> 4a9839a21d7fd95641ba584396bf328fbc1a96b0
> > > > ==========
> > >
> > > I thought about this more, and I think we should probably try to
> implement
> > this
> > > without Mojo. Even though Mojo is pretty lightweight, having one
> message
> pipe
> > > per process recording these UMAs doesn't seem a bit heavy. Assuming the
> shmem
> > > UMAs aren't sharing segments between renderers, we can just record the
> UMA
> > data
> > > in the shared memory segment, with an additional int32 of storage for
> > > single-sample histograms: this holds the 'pending' value. This pending
> value
> > can
> > > be explicitly flushed via destructor invocation, or it can be
> implicitly
> > flushed
> > > when the renderer process exits.
> >
> > Findit suggests this CL added a flaky test
> > "SingleSampleMetricsFactoryImplTest.MultithreadedMetrics" according to
> analysis
> >
> https://findit-for-me.appspot.com/waterfall/flake?key=
> ag9zfmZpbmRpdC1mb3ItbWVyxgELEhdNYXN0ZXJGbGFrZUFuYWx5c2lzUm9v
> dCKPAWNocm9taXVtLmxpbnV4L0FuZHJvaWQgVGVzdHMvNDEzNTUvY29tcG9u
> ZW50c191bml0dGVzdHMgb24gQW5kcm9pZC9VMmx1WjJ4bFUyRnRjR3hsVFdW
> MGNtbGpjMFpoWTNSdmNubEpiWEJzVkdWemRDNU5kV3gwYVhSb2NtVmhaR1Zr
> VFdWMGNtbGpjdz09DAsSE01hc3RlckZsYWtlQW5hbHlzaXMYAQw
> >
> > Can someone please help confirm?
> >
> > Thanks,
> > Jeff on behalf of Findit team
>
> dalecurtis is OOO. Are there features that are blocked on this CL? If not,
> maybe
> we should consider reverting given the size issues with cronet?
>
> https://codereview.chromium.org/2687583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xunjieli, 2017-05-05 19:02:37]

> > dalecurtis is OOO. Are there features that are blocked on this CL? If not,
> > maybe
> > we should consider reverting given the size issues with cronet?
> >
> > https://codereview.chromium.org/2687583002/
> >

Reverting SGTM. I filed crbug.com/718833 for the Cronet issue. Applying
https://codereview.chromium.org/2861153002/ when relanding will fix the binary
size regression.

[Alexei Svitkine (slow), 2017-05-05 19:23:54]

On 2017/05/05 19:02:37, xunjieli wrote:
> > > dalecurtis is OOO. Are there features that are blocked on this CL? If not,
> > > maybe
> > > we should consider reverting given the size issues with cronet?
> > >
> > > https://codereview.chromium.org/2687583002/
> > >
> 
> Reverting SGTM. I filed crbug.com/718833 for the Cronet issue. Applying
> https://codereview.chromium.org/2861153002/ when relanding will fix the binary
> size regression.

Ok, I'll fire off a revert.

[Alexei Svitkine (slow), 2017-05-05 19:24:42]

A revert of this CL (patchset #15 id:360001) has been created in
https://codereview.chromium.org/2862333002/ by asvitkine@chromium.org.

The reason for reverting is: Flaky
SingleSampleMetricsFactoryImplTest.MultithreadedMetrics test and cronet size
regression..

[DaleCurtis, 2017-05-16 22:59:28]

On 2017/05/05 at 19:24:42, asvitkine wrote:
> A revert of this CL (patchset #15 id:360001) has been created in
https://codereview.chromium.org/2862333002/ by asvitkine@chromium.org.
> 
> The reason for reverting is: Flaky
SingleSampleMetricsFactoryImplTest.MultithreadedMetrics test and cronet size
regression..

Sorry about the trouble here. Will take a look at the flaky case today.

dcheng@ I think using shmem would be more efficient, but doing this in the
context of the existing metrics harness is non-trivial, see the earliest
messages on this thread for discussion around this. If the opinion of the
metrics folk has changed since then or they've had a eureka moment, happy to
reconsider. Since these will be rarely used, the overhead of the mojo channel is
negligible in aggregate; so I think until we start seeing more than a handful of
these metrics trying to fit this into the shmem path is premature. I'll be in
Kirkland tomorrow if you'd like to chat more.