media: Add UMA and trace events for CDM creation.

media: Add UMA and trace events for CDM creation.

BUG=512105
TEST=Manually tested by checking about://tracing and about://histograms.

Committed: https://crrev.com/4456541765f3a674eeac2d51c9da30f163bf2f77
Cr-Commit-Position: refs/heads/master@{#339779}

[xhwang, 2015-07-20 20:27:24]

xhwang@chromium.org changed reviewers:
+ asvitkine@chromium.org, ddorwin@chromium.org

[xhwang, 2015-07-20 20:27:24]

ddorwin: Please review from media's perspective.
asvitkine: Please review from UMA's perspective.

[Alexei Svitkine (slow), 2015-07-20 20:53:13]

lgtm

[ddorwin, 2015-07-20 21:35:22]

Just nits.

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.cc:24: const char kCdmCreationTimeUMAName[] =
"CreateCdm";
nit: CreateCdm seems very implementation-specific. What does it mean to "create
a CDM?" CdmCreationTime? Really it's the CdmInstantiationTime, but that's wordy.

Does "time" usually mean "duration" in UMAs?

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.h:138: void
ReportCdmCreationTimeUMA(base::TimeDelta cdm_creation_time);
static? At least const?

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.h:138: void
ReportCdmCreationTimeUMA(base::TimeDelta cdm_creation_time);
nit: "Time" makes it sound like, for example, the UTC time rather than duration.
See also the UMA naming comment.

Naming the variable "duration" would be at least an improvement even if the
function is harder to name.

Also, we probably don't need the "cdm_creation_" part as it's redundant.

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:16029: +<histogram
name="Media.EME.Widevine.CreateCdm" units="milliseconds">
Other key systems will be logged but not have a description, right? Is that
acceptable? I don't want to unnecessarily clutter this, but I don't want to
orphan values either if that's not acceptable.

[xhwang, 2015-07-20 22:10:55]

comments addressed

[xhwang, 2015-07-20 22:11:57]

ddorwin: PTAL again.
asvitkine: Please see a question regarding reports not covered by
histograms.xml.

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
File media/blink/cdm_session_adapter.cc (right):

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.cc:24: const char kCdmCreationTimeUMAName[] =
"CreateCdm";
On 2015/07/20 21:35:21, ddorwin wrote:
> nit: CreateCdm seems very implementation-specific. What does it mean to
"create
> a CDM?" CdmCreationTime? Really it's the CdmInstantiationTime, but that's
wordy.
> 
> Does "time" usually mean "duration" in UMAs?

As discussed offline, we'll use "CreateCdmTime" in UMA name and
"TimeToCreateCdm" in code.

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
File media/blink/cdm_session_adapter.h (right):

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.h:138: void
ReportCdmCreationTimeUMA(base::TimeDelta cdm_creation_time);
On 2015/07/20 21:35:21, ddorwin wrote:
> static?

Can't be static since we need GetKeySystemUMAPrefix().

> At least const?

Done.

https://codereview.chromium.org/1241263002/diff/1/media/blink/cdm_session_ada...
media/blink/cdm_session_adapter.h:138: void
ReportCdmCreationTimeUMA(base::TimeDelta cdm_creation_time);
On 2015/07/20 21:35:21, ddorwin wrote:
> nit: "Time" makes it sound like, for example, the UTC time rather than
duration.
> See also the UMA naming comment.
> 
> Naming the variable "duration" would be at least an improvement even if the
> function is harder to name.

See the reply about the UMA naming.

> Also, we probably don't need the "cdm_creation_" part as it's redundant.

Done.

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:16029: +<histogram
name="Media.EME.Widevine.CreateCdm" units="milliseconds">
On 2015/07/20 21:35:21, ddorwin wrote:
> Other key systems will be logged but not have a description, right? Is that
> acceptable? I don't want to unnecessarily clutter this, but I don't want to
> orphan values either if that's not acceptable.

Correct. Widevine is the only case that we are interested in.

I *think* it's acceptable. The data for other key systems will be kept on the
server but not displayed. If we ever want to see those data, we can add an entry
in this file. +asvitkine to confirm.

[ddorwin, 2015-07-20 22:17:01]

lgtm

[Alexei Svitkine (slow), 2015-07-21 17:13:48]

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:16029: +<histogram
name="Media.EME.Widevine.CreateCdm" units="milliseconds">
On 2015/07/20 22:11:57, xhwang wrote:
> On 2015/07/20 21:35:21, ddorwin wrote:
> > Other key systems will be logged but not have a description, right? Is that
> > acceptable? I don't want to unnecessarily clutter this, but I don't want to
> > orphan values either if that's not acceptable.
> 
> Correct. Widevine is the only case that we are interested in.
> 
> I *think* it's acceptable. The data for other key systems will be kept on the
> server but not displayed. If we ever want to see those data, we can add an
entry
> in this file. +asvitkine to confirm.
> 

From UMA point of view, this is not ideal. Preferably, we would like to be able
to decode all histogram hashes on the server side. So if we know what the others
are already, it's best to document them in this CL. You can use the
histogram_suffixes construct to avoid too much duplication in the XML (see the
comments at the top of the file for some examples of how to).

Alternatively, if you really don't care about the data for the other ones, then
the other thing you could do is add a conditional to the code to only log this
for Widevine.

[xhwang, 2015-07-21 22:11:23]

comments addressed

[xhwang, 2015-07-21 22:14:22]

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241263002/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:16029: +<histogram
name="Media.EME.Widevine.CreateCdm" units="milliseconds">
On 2015/07/21 17:13:48, Alexei Svitkine wrote:
> On 2015/07/20 22:11:57, xhwang wrote:
> > On 2015/07/20 21:35:21, ddorwin wrote:
> > > Other key systems will be logged but not have a description, right? Is
that
> > > acceptable? I don't want to unnecessarily clutter this, but I don't want
to
> > > orphan values either if that's not acceptable.
> > 
> > Correct. Widevine is the only case that we are interested in.
> > 
> > I *think* it's acceptable. The data for other key systems will be kept on
the
> > server but not displayed. If we ever want to see those data, we can add an
> entry
> > in this file. +asvitkine to confirm.
> > 
> 
> From UMA point of view, this is not ideal. Preferably, we would like to be
able
> to decode all histogram hashes on the server side. So if we know what the
others
> are already, it's best to document them in this CL. You can use the
> histogram_suffixes construct to avoid too much duplication in the XML (see the
> comments at the top of the file for some examples of how to).
> 
> Alternatively, if you really don't care about the data for the other ones,
then
> the other thing you could do is add a conditional to the code to only log this
> for Widevine.

Done. 

histogram_suffixes doesn't fit cleanly in this case. I do want to keep
Media.EME.Widevine.* pattern, which most existing EME metrics use today. The
ordering="prefix" trick doesn't work as well as Media.EME. has two dots....

[xhwang, 2015-07-21 22:14:28]

The CQ bit was checked by xhwang@chromium.org

[xhwang, 2015-07-21 22:14:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, ddorwin@chromium.org
Link to the patchset: https://codereview.chromium.org/1241263002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-07-21 22:14:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241263002/40001

[commit-bot: I haz the power, 2015-07-21 23:37:48]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-21 23:38:46]

Patchset 3 (id:??) landed as
https://crrev.com/4456541765f3a674eeac2d51c9da30f163bf2f77
Cr-Commit-Position: refs/heads/master@{#339779}