Add one extra UMA for video casting

- Include the media type of the video that is being flinged
- Add two new (fairly common) containers to the MediaContainers enum

 Views that include pre-M34 data will categorize these values (dash and smooth stream) in the "Other" bucket. 

BUG=339093
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=250300

[Miguel Garcia, 2014-01-30 18:08:41]

Mark for the histograms.
xhwang for the new enum values in media.

[xhwang, 2014-01-31 00:27:41]

Please fix the issue title "pretty print".

Can you show me where we report the UMA for these new container types? Thanks!

https://codereview.chromium.org/133483003/diff/1/media/base/container_names.h
File media/base/container_names.h (right):

https://codereview.chromium.org/133483003/diff/1/media/base/container_names.h...
media/base/container_names.h:60: CONTAINER_SMOOTHSTREAM,      // SmoothStreaming
comment indent is off

[Miguel Garcia, 2014-01-31 12:19:02]

https://codereview.chromium.org/133483003/diff/1/media/base/container_names.h
File media/base/container_names.h (right):

https://codereview.chromium.org/133483003/diff/1/media/base/container_names.h...
media/base/container_names.h:60: CONTAINER_SMOOTHSTREAM,      // SmoothStreaming
On 2014/01/31 00:27:42, xhwang wrote:
> comment indent is off

Done.

[Miguel Garcia, 2014-01-31 12:20:44]

Thanks for the review!


On 2014/01/31 00:27:41, xhwang wrote:
> Please fix the issue title "pretty print".

Done
> 
> Can you show me where we report the UMA for these new container types? Thanks!

They are triggered in java from our internal repo.

See 
https://chrome-internal-review.googlesource.com/#/c/153109/1

[xhwang, 2014-01-31 20:04:25]

On 2014/01/31 12:20:44, Miguel Garcia wrote:
> Thanks for the review!
> 
> 
> On 2014/01/31 00:27:41, xhwang wrote:
> > Please fix the issue title "pretty print".
> 
> Done
> > 
> > Can you show me where we report the UMA for these new container types?
Thanks!
> 
> They are triggered in java from our internal repo.
> 
> See 
> https://chrome-internal-review.googlesource.com/#/c/153109/1

lgtm % nits

[xhwang, 2014-01-31 20:04:37]

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:211:
actions.add('Cast_Sender_YouTubeDeviceSelected');
can you help fix the order here?

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:213:
actions.add('Cast_Sender_CastMediaType');
I believe these actions are added in lexicographical order.

[Miguel Garcia, 2014-02-03 09:56:18]

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
File tools/metrics/actions/extract_actions.py (right):

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:211:
actions.add('Cast_Sender_YouTubeDeviceSelected');
On 2014/01/31 20:04:39, xhwang wrote:
> can you help fix the order here?

Done.

https://codereview.chromium.org/133483003/diff/20001/tools/metrics/actions/ex...
tools/metrics/actions/extract_actions.py:213:
actions.add('Cast_Sender_CastMediaType');
On 2014/01/31 20:04:39, xhwang wrote:
> I believe these actions are added in lexicographical order.

Done.

[Miguel Garcia, 2014-02-03 09:56:56]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-02-03 09:57:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/133483003/100001

[commit-bot: I haz the power, 2014-02-03 10:31:58]

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

[commit-bot: I haz the power, 2014-02-03 10:31:59]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-02-03 10:32:05]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 10:32:09]

CQ bit was unchecked on CL. Ignoring.

[Mark P, 2014-02-03 15:57:07]

https://codereview.chromium.org/133483003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/133483003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27793: +  <int value="38" label="DASH"/>
Are these new values things that were previously counted, just counted as
value=0 UNKNOWN?

If so, standard good practices means that everytime you do this, you should
change the histogram name.  Otherwise the "Everything" view on the dashboard
will be mixing two different interpretations of the data and make no sense.

[Miguel Garcia, 2014-02-04 13:31:18]

https://codereview.chromium.org/133483003/diff/100001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/133483003/diff/100001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:27793: +  <int value="38" label="DASH"/>
Looking at the code they would have been categorized as UNKNOWN so I changed the
name of the metric. Thanks for spotting this.

On 2014/02/03 15:57:08, Mark P wrote:
> Are these new values things that were previously counted, just counted as
> value=0 UNKNOWN?
> 
> If so, standard good practices means that everytime you do this, you should
> change the histogram name.  Otherwise the "Everything" view on the dashboard
> will be mixing two different interpretations of the data and make no sense.

[xhwang, 2014-02-04 17:22:24]

Mark's point makes sense. But I am concerned that in the future, whenever we
want to add a new container type to report, we need to update the name of the
histogram again. I am not too worried about "mixing the two different
interpretations" in the "everything" view. People can always choose the correct
timeframe to see the histogram they want to see.

That being said, I am not an expert in UMA. Adding jar@ for more comments.

[Mark P, 2014-02-04 18:28:39]

On 2014/02/04 17:22:24, xhwang wrote:
> Mark's point makes sense. But I am concerned that in the future, whenever we
> want to add a new container type to report, we need to update the name of the
> histogram again. I am not too worried about "mixing the two different
> interpretations" in the "everything" view. People can always choose the
correct
> timeframe to see the histogram they want to see.

If you really want to avoid changing the histogram name regularly,
another possibility is adding comments in histograms.xml,
making it clear that the "Everything" view should not be trusted,
and identifying for each new histogram  bucket at exactly what browser
version that the bucket started being emitted to.  That way a user
of the dashboard will know that, for example, if the user wants the
distribution of stream types and wants to include, say, SmoothStream
in that distribution, the user needs to look at least at version X.Y.Z.
(This will save the user either a lot of guess and checking or some
code archaeology to determine the proper version cutoff.)
Frankly, this tons-of-comments seems like a pain, and it still lets
people misinterpret the "Everything" view when they don't read the
histogram description (which I'm afraid happens more often than
you'd hope).

--mark

> That being said, I am not an expert in UMA. Adding jar@ for more comments.

[xhwang, 2014-02-04 19:19:32]

On 2014/02/04 18:28:39, Mark P wrote:
> On 2014/02/04 17:22:24, xhwang wrote:
> > Mark's point makes sense. But I am concerned that in the future, whenever we
> > want to add a new container type to report, we need to update the name of
the
> > histogram again. I am not too worried about "mixing the two different
> > interpretations" in the "everything" view. People can always choose the
> correct
> > timeframe to see the histogram they want to see.
> 
> If you really want to avoid changing the histogram name regularly,
> another possibility is adding comments in histograms.xml,
> making it clear that the "Everything" view should not be trusted,
> and identifying for each new histogram  bucket at exactly what browser
> version that the bucket started being emitted to.  That way a user
> of the dashboard will know that, for example, if the user wants the
> distribution of stream types and wants to include, say, SmoothStream
> in that distribution, the user needs to look at least at version X.Y.Z.
> (This will save the user either a lot of guess and checking or some
> code archaeology to determine the proper version cutoff.)
> Frankly, this tons-of-comments seems like a pain, and it still lets
> people misinterpret the "Everything" view when they don't read the
> histogram description (which I'm afraid happens more often than
> you'd hope).
> 
> --mark

Extra comments sgtm. For this particular histogram, we initially added support
for common/popular container types. Then we want to monitor how many "unknown"
container types we have, and gradually add support for those. As part of this
process, we'll need to expand the histogram gradually. That's why I was worried
about changing the histogram names when adding new container types. It's
actually a feature to see the trend of Media.DetectedContainer.Unknown overtime
to see the our improved coverage of container types. If we keep changing the
histogram name, this feature will be broken.

 
> > That being said, I am not an expert in UMA. Adding jar@ for more comments.

[Miguel Garcia, 2014-02-04 19:20:45]

I am ok with either choice but I guess I can just proceed with the new
name for this one? Neither of you seem too convinced about the extra
comments.

On Tue, Feb 4, 2014 at 6:28 PM,  <mpearson@chromium.org> wrote:
> On 2014/02/04 17:22:24, xhwang wrote:
>>
>> Mark's point makes sense. But I am concerned that in the future, whenever
>> we
>> want to add a new container type to report, we need to update the name of
>> the
>> histogram again. I am not too worried about "mixing the two different
>> interpretations" in the "everything" view. People can always choose the
>
> correct
>>
>> timeframe to see the histogram they want to see.
>
>
> If you really want to avoid changing the histogram name regularly,
> another possibility is adding comments in histograms.xml,
> making it clear that the "Everything" view should not be trusted,
> and identifying for each new histogram  bucket at exactly what browser
> version that the bucket started being emitted to.  That way a user
> of the dashboard will know that, for example, if the user wants the
> distribution of stream types and wants to include, say, SmoothStream
> in that distribution, the user needs to look at least at version X.Y.Z.
> (This will save the user either a lot of guess and checking or some
> code archaeology to determine the proper version cutoff.)
> Frankly, this tons-of-comments seems like a pain, and it still lets
> people misinterpret the "Everything" view when they don't read the
> histogram description (which I'm afraid happens more often than
> you'd hope).
>
> --mark
>
>
>> That being said, I am not an expert in UMA. Adding jar@ for more comments.
>
>
>
>
> https://codereview.chromium.org/133483003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xhwang, 2014-02-04 19:23:15]

On 2014/02/04 19:20:45, Miguel Garcia wrote:
> I am ok with either choice but I guess I can just proceed with the new
> name for this one? Neither of you seem too convinced about the extra
> comments.

Please see my comments. I think I am in favor of extra comments than new
histogram names. 
> 
> On Tue, Feb 4, 2014 at 6:28 PM,  <mailto:mpearson@chromium.org> wrote:
> > On 2014/02/04 17:22:24, xhwang wrote:
> >>
> >> Mark's point makes sense. But I am concerned that in the future, whenever
> >> we
> >> want to add a new container type to report, we need to update the name of
> >> the
> >> histogram again. I am not too worried about "mixing the two different
> >> interpretations" in the "everything" view. People can always choose the
> >
> > correct
> >>
> >> timeframe to see the histogram they want to see.
> >
> >
> > If you really want to avoid changing the histogram name regularly,
> > another possibility is adding comments in histograms.xml,
> > making it clear that the "Everything" view should not be trusted,
> > and identifying for each new histogram  bucket at exactly what browser
> > version that the bucket started being emitted to.  That way a user
> > of the dashboard will know that, for example, if the user wants the
> > distribution of stream types and wants to include, say, SmoothStream
> > in that distribution, the user needs to look at least at version X.Y.Z.
> > (This will save the user either a lot of guess and checking or some
> > code archaeology to determine the proper version cutoff.)
> > Frankly, this tons-of-comments seems like a pain, and it still lets
> > people misinterpret the "Everything" view when they don't read the
> > histogram description (which I'm afraid happens more often than
> > you'd hope).
> >
> > --mark
> >
> >
> >> That being said, I am not an expert in UMA. Adding jar@ for more comments.
> >
> >
> >
> >
> > https://codereview.chromium.org/133483003/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[jar (doing other things), 2014-02-04 19:57:38]

On 2014/02/04 17:22:24, xhwang wrote:
> Mark's point makes sense. But I am concerned that in the future, whenever we
> want to add a new container type to report, we need to update the name of the
> histogram again. I am not too worried about "mixing the two different
> interpretations" in the "everything" view. People can always choose the
correct
> timeframe to see the histogram they want to see.
> 
> That being said, I am not an expert in UMA. Adding jar@ for more comments.

This CL skirts the line for the justification of changing the name of the
histogram, vs adding new enums (which you did correctly do at the end of the
list!).

It is *critical* to change the name when old slots change their meaning to
completely new (or permuted) names, or when the numerical bounds on exponential
histogram get changed (so that numerical cutoffs shift all over the place).  

In this case, you are seemingly expanding out the OTHER enumerated category, and
giving more precision (a break-out for some entries formerly in OTHER).  Folks
looking at old data (dates) won't be very confused (won't see new buckets). 
Folks looking at new data might be somewhat confused seeing OTHER counts from
old browsers, and specific (new) counts in the new enum buckets from new
browsers... but that state won't last long....

There was also mention of some other code that processed this histogram, and
would really like to keep lookin' at the old name.

Although I hate usin' comments, the subtlety is probably small and waning, and a
comment would probably carry us across this transition reasonably well.\

I'd vote with the plan mentioned by a few others of keeping the name... and
adding some prose to the description in the XML file warning that "OTHER
category prior to version NNN can sometimes contain the new counts for foo and
goo."

YMMV.... it is not the end of the world to violate the best practice... when you
have some reasons.  ...but it is good to start with best practice and argue from
there ;-).

[Miguel Garcia, 2014-02-10 17:42:12]

Thanks for the detailed feedback I added some comments and reverted the name
change. Could you take another look?


On 2014/02/04 19:57:38, jar wrote:
> On 2014/02/04 17:22:24, xhwang wrote:
> > Mark's point makes sense. But I am concerned that in the future, whenever we
> > want to add a new container type to report, we need to update the name of
the
> > histogram again. I am not too worried about "mixing the two different
> > interpretations" in the "everything" view. People can always choose the
> correct
> > timeframe to see the histogram they want to see.
> > 
> > That being said, I am not an expert in UMA. Adding jar@ for more comments.
> 
> This CL skirts the line for the justification of changing the name of the
> histogram, vs adding new enums (which you did correctly do at the end of the
> list!).
> 
> It is *critical* to change the name when old slots change their meaning to
> completely new (or permuted) names, or when the numerical bounds on
exponential
> histogram get changed (so that numerical cutoffs shift all over the place).  
> 
> In this case, you are seemingly expanding out the OTHER enumerated category,
and
> giving more precision (a break-out for some entries formerly in OTHER).  Folks
> looking at old data (dates) won't be very confused (won't see new buckets). 
> Folks looking at new data might be somewhat confused seeing OTHER counts from
> old browsers, and specific (new) counts in the new enum buckets from new
> browsers... but that state won't last long....
> 
> There was also mention of some other code that processed this histogram, and
> would really like to keep lookin' at the old name.
> 
> Although I hate usin' comments, the subtlety is probably small and waning, and
a
> comment would probably carry us across this transition reasonably well.\
> 
> I'd vote with the plan mentioned by a few others of keeping the name... and
> adding some prose to the description in the XML file warning that "OTHER
> category prior to version NNN can sometimes contain the new counts for foo and
> goo."
> 
> YMMV.... it is not the end of the world to violate the best practice... when
you
> have some reasons.  ...but it is good to start with best practice and argue
from
> there ;-).

[Mark P, 2014-02-10 17:45:15]

https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:28122: + values in the "Other" bucket-->
Please put a comment like this in the description/summary so people actually see
it, not as an comment in the XML source code itself.

[Miguel Garcia, 2014-02-10 18:17:41]

On 2014/02/10 17:45:15, Mark P wrote:
>
https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:28122: + values in the "Other"
bucket-->
> Please put a comment like this in the description/summary so people actually
see
> it, not as an comment in the XML source code itself.

Done

[Mark P, 2014-02-10 18:28:10]

On 2014/02/10 18:17:41, Miguel Garcia wrote:
> On 2014/02/10 17:45:15, Mark P wrote:
> >
>
https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
> > File tools/metrics/histograms/histograms.xml (right):
> > 
> >
>
https://codereview.chromium.org/133483003/diff/380001/tools/metrics/histogram...
> > tools/metrics/histograms/histograms.xml:28122: + values in the "Other"
> bucket-->
> > Please put a comment like this in the description/summary so people actually
> see
> > it, not as an comment in the XML source code itself.
> 
> Done

I don't see it in the most recent patchset.

--mark

[Mark P, 2014-02-10 22:18:13]

lgtm

[Miguel Garcia, 2014-02-10 22:30:58]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-02-10 22:32:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/133483003/470001

[commit-bot: I haz the power, 2014-02-10 22:58:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/133483003/470001

[commit-bot: I haz the power, 2014-02-10 23:53:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miguelg@chromium.org/133483003/470001

[commit-bot: I haz the power, 2014-02-11 02:51:05]

Change committed as 250300