Add a comment clarifying how UkmEntryBuilder::AddMetric works.

Add a comment clarifying how UkmEntryBuilder::AddMetric works.

Repeated calls to AddMetric with the same metric name are additive, rather than replacing.

[cbentzel, 2017-04-14 16:51:03]

cbentzel@chromium.org changed reviewers:
+ holte@chromium.org

[cbentzel, 2017-04-14 16:51:04]

I found this aspect unclear.

Was also wondering if there were plans to add unit tests to this directory.

[cbentzel, 2017-04-14 16:51:31]

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

[commit-bot: I haz the power, 2017-04-14 16:51:39]

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-04-14 18:02:48]

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

[commit-bot: I haz the power, 2017-04-14 18:02:49]

Dry run: This issue passed the CQ dry run.

[Steven Holte, 2017-04-17 20:48:31]

I think I may actually prefer to make recording the same metric multiple times
on a single Entry explicitly disallowed.  The main use cases for this generally
seem to be better covered by recording multiple Entries.

[cbentzel, 2017-04-19 12:00:50]

On 2017/04/17 20:48:31, Steven Holte wrote:
> I think I may actually prefer to make recording the same metric multiple times
> on a single Entry explicitly disallowed.  The main use cases for this
generally
> seem to be better covered by recording multiple Entries.

I'll leave that up to you - but in either case it feels like the behavior should
be explicitly commented on and ideally unit-tested as it's currently ambiguous.


I also looked at some of the data pipeline code and it looked like this was
explicitly supported.

[cbentzel, 2017-04-23 23:34:00]

On 2017/04/19 12:00:50, cbentzel wrote:
> On 2017/04/17 20:48:31, Steven Holte wrote:
> > I think I may actually prefer to make recording the same metric multiple
times
> > on a single Entry explicitly disallowed.  The main use cases for this
> generally
> > seem to be better covered by recording multiple Entries.
> 
> I'll leave that up to you - but in either case it feels like the behavior
should
> be explicitly commented on and ideally unit-tested as it's currently
ambiguous.
> 
> 
> I also looked at some of the data pipeline code and it looked like this was
> explicitly supported.

I wanted to bring up one potential use case to see how you think about layout
for UKM entries.

There's interest in bringing UseCounter statistics to UKM (see
http://www.chromium.org/blink/platform-predictability/compat-tools about it, but
basically measures blink features used normalized by page load counts).

For UKM, my initial thought would be that we would repeatedly do something like
AddMetric("UseCounter", <integral feature id>) for a single entry.

Are you proposing doing multiple entries for that use case?

[cbentzel, 2017-04-23 23:34:18]

Description was changed from

==========
Add a comment clarifying how UkmEntryBuilder::AddMetric works.

Repeated calls to AddMetric with the same metric name are additive, rather than
replacing.
==========

to

==========
Add a comment clarifying how UkmEntryBuilder::AddMetric works.

Repeated calls to AddMetric with the same metric name are additive, rather than
replacing.
==========

[cbentzel, 2017-04-23 23:34:36]

On 2017/04/23 23:34:00, cbentzel wrote:
> On 2017/04/19 12:00:50, cbentzel wrote:
> > On 2017/04/17 20:48:31, Steven Holte wrote:
> > > I think I may actually prefer to make recording the same metric multiple
> times
> > > on a single Entry explicitly disallowed.  The main use cases for this
> > generally
> > > seem to be better covered by recording multiple Entries.
> > 
> > I'll leave that up to you - but in either case it feels like the behavior
> should
> > be explicitly commented on and ideally unit-tested as it's currently
> ambiguous.
> > 
> > 
> > I also looked at some of the data pipeline code and it looked like this was
> > explicitly supported.
> 
> I wanted to bring up one potential use case to see how you think about layout
> for UKM entries.
> 
> There's interest in bringing UseCounter statistics to UKM (see
> http://www.chromium.org/blink/platform-predictability/compat-tools about it,
but
> basically measures blink features used normalized by page load counts).
> 
> For UKM, my initial thought would be that we would repeatedly do something
like
> AddMetric("UseCounter", <integral feature id>) for a single entry.
> 
> Are you proposing doing multiple entries for that use case?

Also cc'ed rbyers in case he is interested in this.

[Steven Holte, 2017-04-24 19:47:53]

On 2017/04/23 23:34:36, cbentzel wrote:
> On 2017/04/23 23:34:00, cbentzel wrote:
> > On 2017/04/19 12:00:50, cbentzel wrote:
> > > On 2017/04/17 20:48:31, Steven Holte wrote:
> > > > I think I may actually prefer to make recording the same metric multiple
> > times
> > > > on a single Entry explicitly disallowed.  The main use cases for this
> > > generally
> > > > seem to be better covered by recording multiple Entries.
> > > 
> > > I'll leave that up to you - but in either case it feels like the behavior
> > should
> > > be explicitly commented on and ideally unit-tested as it's currently
> > ambiguous.
> > > 
> > > 
> > > I also looked at some of the data pipeline code and it looked like this
was
> > > explicitly supported.
> > 
> > I wanted to bring up one potential use case to see how you think about
layout
> > for UKM entries.
> > 
> > There's interest in bringing UseCounter statistics to UKM (see
> > http://www.chromium.org/blink/platform-predictability/compat-tools about it,
> but
> > basically measures blink features used normalized by page load counts).
> > 
> > For UKM, my initial thought would be that we would repeatedly do something
> like
> > AddMetric("UseCounter", <integral feature id>) for a single entry.
> > 
> > Are you proposing doing multiple entries for that use case?
> 
> Also cc'ed rbyers in case he is interested in this.

The main issue with repeating metrics is that they are not very extensible with
additional information, e.g. if you record a page using 3 features by recording
an Entry with 3 UseCounter values [Feature1, Feature2, Feature3] it's difficult
to later add more information like "which/what kind of frame Feature2 was used
in" without repeating the information, or having order dependence between
repeating fields, so we want to discourage that.

The most common solution is usually to repeat the Entry instead.  So you might
record a FeatureUsage Event with Feature as the metric.  This has a small amount
of overhead (maybe 2x size), but let's the event be extended with related
metrics later.  That's usually not going to be the difference between a metric
using a reasonable amount of bandwidth or not, especially compared to other
mitigating strategies like sampling / summarizing, or in the case of UseCounter,
packing features into bit vectors.

[cbentzel, 2017-04-28 16:06:27]

On 2017/04/24 19:47:53, Steven Holte wrote:
> On 2017/04/23 23:34:36, cbentzel wrote:
> > On 2017/04/23 23:34:00, cbentzel wrote:
> > > On 2017/04/19 12:00:50, cbentzel wrote:
> > > > On 2017/04/17 20:48:31, Steven Holte wrote:
> > > > > I think I may actually prefer to make recording the same metric
multiple
> > > times
> > > > > on a single Entry explicitly disallowed.  The main use cases for this
> > > > generally
> > > > > seem to be better covered by recording multiple Entries.
> > > > 
> > > > I'll leave that up to you - but in either case it feels like the
behavior
> > > should
> > > > be explicitly commented on and ideally unit-tested as it's currently
> > > ambiguous.
> > > > 
> > > > 
> > > > I also looked at some of the data pipeline code and it looked like this
> was
> > > > explicitly supported.
> > > 
> > > I wanted to bring up one potential use case to see how you think about
> layout
> > > for UKM entries.
> > > 
> > > There's interest in bringing UseCounter statistics to UKM (see
> > > http://www.chromium.org/blink/platform-predictability/compat-tools about
it,
> > but
> > > basically measures blink features used normalized by page load counts).
> > > 
> > > For UKM, my initial thought would be that we would repeatedly do something
> > like
> > > AddMetric("UseCounter", <integral feature id>) for a single entry.
> > > 
> > > Are you proposing doing multiple entries for that use case?
> > 
> > Also cc'ed rbyers in case he is interested in this.
> 
> The main issue with repeating metrics is that they are not very extensible
with
> additional information, e.g. if you record a page using 3 features by
recording
> an Entry with 3 UseCounter values [Feature1, Feature2, Feature3] it's
difficult
> to later add more information like "which/what kind of frame Feature2 was used
> in" without repeating the information, or having order dependence between
> repeating fields, so we want to discourage that.
> 
> The most common solution is usually to repeat the Entry instead.  So you might
> record a FeatureUsage Event with Feature as the metric.  This has a small
amount
> of overhead (maybe 2x size), but let's the event be extended with related
> metrics later.  That's usually not going to be the difference between a metric
> using a reasonable amount of bandwidth or not, especially compared to other
> mitigating strategies like sampling / summarizing, or in the case of
UseCounter,
> packing features into bit vectors.

Thanks. I'm going to close this CL as it sounds like there is still ambiguity
around expected behavior here, but you are leaning away from the behavior that I
described.