Translate: UMA for "Show original"

Translate: UMA for "Show original"

BUG=242145
TBR=jar@chromium.org

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

[Takashi Toyoshima, 2013-05-22 03:39:33]

Hi Ilya,
Can you take a look?

[Takashi Toyoshima, 2013-05-22 23:53:10]

ping

[jar (doing other things), 2013-05-23 01:34:40]

SteveT/Alexei: Please review and comment on this CL.  It will be good practice
for the impending ownership.

Thanks,

Jim

[Alexei Svitkine (slow), 2013-05-23 14:16:13]

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

https://codereview.chromium.org/15646003/diff/1/tools/metrics/histograms/hist...
tools/metrics/histograms/histograms.xml:8585: <histogram
name="Translate.RevertTranslation">
Have you considered changing Translate.Translate into an enumerated histogram
instead of introducing a new one?

The enumerated histogram could then cover all the different actions a user can
perform on the dialog, with different enum values for each.

(Incidentally, I'm not seeing Translate.Translate histogram in this file; is it
in the internal histograms.xml file?)

[Takashi Toyoshima, 2013-05-23 14:52:54]

Yeah, other metrics in this file were introduced long time ago by another
person.
Also they are defined in the internal one.

When we use enum for Translate.Translate, we should apply to not only it, but
also all metrics in this file. But, it make it difficult to get existing
timeline graph. So, I want to keep consistency in this file.

[Alexei Svitkine (slow), 2013-05-23 15:51:43]

On 2013/05/23 14:52:54, Takashi Toyoshima (chromium) wrote:
> Yeah, other metrics in this file were introduced long time ago by another
> person.
> Also they are defined in the internal one.
> 
> When we use enum for Translate.Translate, we should apply to not only it, but
> also all metrics in this file. But, it make it difficult to get existing
> timeline graph. So, I want to keep consistency in this file.

I don't think all the metrics should be in a single enum; only the ones that are
in the same category of metrics - e.g. "actions that can be performed on the
translate infobar that are mutually exclusive".

I think the timeline graphs now support enumerated histograms. So if you keep
the value of Translate.Translate and add this one as another enum to that
histogram, it should continue to keep working for that metric.

[Takashi Toyoshima, 2013-05-23 16:19:47]

All metrics in this file is mapped to each button in Translate infobars. So,
Translate.Translate is not special against Translate.Revert. We just find that
only revert button missed to report UMA.

[Alexei Svitkine (slow), 2013-05-23 16:39:42]

Okay, fair enough. LGTM.

[jar (doing other things), 2013-05-23 17:20:00]

The following defends and expands on Alexei's concerns.

https://codereview.chromium.org/15646003/diff/1/chrome/browser/translate/tran...
File chrome/browser/translate/translate_infobar_delegate.cc (right):

https://codereview.chromium.org/15646003/diff/1/chrome/browser/translate/tran...
chrome/browser/translate/translate_infobar_delegate.cc:101:
UMA_HISTOGRAM_COUNTS(kRevertTranslation, 1);
This pattern of trying to use histograms as single bucket counters is actually
problematic in several ways.

First of all, you really want to see the counts next to each other, so using an
enumerated histogram gets you these counters visualized exactly as you actually
need them: Several counters next to one another, providing normalization factors
for each other.

Second, it is wasteful of both run-time memory, and backend dashboard-pipeline
memory.  The cost of "an additional enumerated bucket" is about 12 bytes in the
browser.  Adding another histogram is surely in the range of a few hundred bytes
minimum to the browser, and a similar jump in cost/complexity in the processing
pipeline :-(.

Lastly, you've accidentally used UMA_HISTOGRAM_COUNTS(), which creates space for
50 buckets.  That costs about 500-600 bytes.  Much better in this poor usage
pattern would be UMA_HISOGRAM_BOOLEAN(), which at least only creates one bucket.

Bottom line: You really should use an enumerated histogram.  If you really want
to use up a few KB of RAM space extra, and stay with this pattern, you should at
least use UMA_HISTOGRAM_BOOLEAN("foo", true);

[Takashi Toyoshima, 2013-05-23 17:50:32]

Yes, I used COUNTS here because of very historical reason.

Usually, I love to use enum for histogram. Translate UMAs newly introduced by me
use enum properly.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tra...
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/tr...

By the way, is it possible to change existing bunch of COUNTS(..., 1) pattern
UMAs to use BOOLEAN?
If it is possible and effective, and also server can continue to provide
timeline graph with old data, I'll make a CL for that.

[jar (doing other things), 2013-05-23 23:25:13]

On 2013/05/23 17:50:32, Takashi Toyoshima (chromium) wrote:
> Yes, I used COUNTS here because of very historical reason.
> 
> Usually, I love to use enum for histogram. Translate UMAs newly introduced by
me
> use enum properly.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/tra...
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/tr...
> 
> By the way, is it possible to change existing bunch of COUNTS(..., 1) pattern
> UMAs to use BOOLEAN?
> If it is possible and effective, and also server can continue to provide
> timeline graph with old data, I'll make a CL for that.


Bottom line: Yes.  You can replace with UMA_HISTOGRAMS_BOOLEAN, and the back end
will seemlessly combine them.  Since you passed in a sample of "1" you got
tallied into the 1st bucket, which has a range of [1,2) in *_COUNTS.  If you use
UMA_HISTOGRAM_BOOLEAN, you should pass in a sample of "true," which should also
tally a count into a bucket with a "range" of [1,2).  

The backend won't be able to distinguish the difference (it is not
communicated), and the tallies should fit together beautifully.

[First... to correct myself... BOOLEAN histograms create histograms with
something like 2 or 3 buckets, not "1 bucket."  0th is for false; 1st is for
true, and there may, almost accidentally.. be an overflow bucket... but
hopefully not].

HISTOGRAM_COUNTS creates a histogram with about 50 buckets, where the first
counts the number of samples that have a sample value of 0 (which you didn't
use), and under the covers its "range" is defined to be [-infinity, 1).  The
second ends up with [1,2), since the buckets are spread by roughly the 50th root
of the total dynamic range, which is 1 to 1 million.  

You can take a look at the declaration of UMA_HISTOGRAM_BOOLEAN and
UMA_HISTOGRAM_COUNTS in base/metrics/histogram.h.

[Takashi Toyoshima, 2013-05-27 06:19:21]

jar: I filed a bug for cleanup
https://code.google.com/p/chromium/issues/detail?id=244143
hajimehoshi@ and I started to work on this.

I'll land this as is for now.

[commit-bot: I haz the power, 2013-05-27 06:21:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15646003/30001

[commit-bot: I haz the power, 2013-05-27 06:32:02]

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, 2013-05-27 07:19:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/toyoshim@chromium.org/15646003/30001

[commit-bot: I haz the power, 2013-05-27 13:24:09]

Change committed as 202413