Removing extension name suffix from the metric name in chrome.experimental.metrics

Removing extension name suffix from the metric name in chrome.experimental.metrics

Also removed an obsolete comment.

Rationale:
1. Extension name postfixes make action names less readable than they need to be.
2. There are not so many extensions using the API. To be precise, at the moment there is exactly one (Chrome OS built-in File Browser).
3. Every new metric needs to be registered in a dashboard configuration file anyway. Names are sorted alphabetically there and duplicates can be easily detected.

At the very least we could omit the extension name postfix only for built-in extensions.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110295

[Roger Tawa OOO till Jul 10th, 2011-11-15 12:52:29]

The extension id suffix is a security/usability feature to prevent one
extension from overwriting metrics of chrome itself or other extensions
that also use the metrics API. The suffix cannot be removed unless there is
a solution to this. Exceptions could be made for internal extensions.  Are
you planning to remove this from experimental?
 On Nov 15, 2011 6:35 AM, <kaznacheev@chromium.org> wrote:

> Reviewers: Roger Tawa, hansl,
>
> Description:
> Removing extension name suffix from the metric name in
> chrome.experimental.metrics
>
> Also removed an obsolete comment.
>
> Rationale:
> 1. Extension name postfixes make action names less readable than they need
> to
> be.
> 2. There are not so many extensions using the API. To be precise, at the
> moment
> there is exactly one (Chrome OS built-in File Browser).
> 3. Every new metric needs to be registered in a dashboard configuration
> file
> anyway. Names are sorted alphabetically there and duplicates can be easily
> detected.
>
> At the very least we could omit the extension name postfix only for
> built-in
> extensions.
>
> BUG=
> TEST=
>
>
> Please review this at
http://codereview.chromium.**org/8510046/<http://codereview.chromium.org/8510...
>
> SVN Base:
http://git.chromium.org/git/**chromium.git@trunk<http://git.chromium.org/git/...
>
> Affected files:
>  M chrome/browser/extensions/**extension_metrics_module.cc
>
>
> Index: chrome/browser/extensions/**extension_metrics_module.cc
> diff --git a/chrome/browser/extensions/**extension_metrics_module.cc
> b/chrome/browser/extensions/**extension_metrics_module.cc
> index 8e3428d1e146b8ba8f8d688a4ca313**fff45fad49..**
> 84d81160a4eaaac4c0e1275837a43f**9f56ede0cb 100644
> --- a/chrome/browser/extensions/**extension_metrics_module.cc
> +++ b/chrome/browser/extensions/**extension_metrics_module.cc
> @@ -14,25 +14,6 @@
>  using base::Histogram;
>  using base::LinearHistogram;
>
> -namespace {
> -
> -// Build the full name of a metrics for the given extension.  Each metric
> -// is made up of the unique name within the extension followed by the
> -// extension's id.  This keeps the metrics from one extension unique from
> -// other extensions, as well as those metrics from chrome itself.
> -std::string BuildMetricName(const std::string& name,
> -                            const Extension* extension) {
> -  std::string full_name(name);
> -  full_name += extension->id();
> -  return full_name;
> -}
> -
> -}  // anonymous namespace
> -
> -// These extension function classes are enabled only if the
> -// enable-metrics-extension-api command line switch is used.  Refer to
> -// extension_function_dispatcher.**cc to see how they are enabled.
> -
>  bool MetricsSetEnabledFunction::**RunImpl() {
>   bool enabled = false;
>   EXTENSION_FUNCTION_VALIDATE(**args_->GetBoolean(0, &enabled));
> @@ -56,7 +37,6 @@ bool MetricsRecordUserActionFunctio**n::RunImpl() {
>   std::string name;
>   EXTENSION_FUNCTION_VALIDATE(**args_->GetString(0, &name));
>
> -  name = BuildMetricName(name, GetExtension());
>   UserMetrics::**RecordComputedAction(name);
>   return true;
>  }
> @@ -74,16 +54,15 @@ bool MetricsHistogramHelperFunction**::RecordValue(const
> std::string& name,
>                                                  int max,
>                                                  size_t buckets,
>                                                  int sample) {
> -  std::string full_name = BuildMetricName(name, GetExtension());
>   Histogram* counter;
>   if (type == Histogram::LINEAR_HISTOGRAM) {
> -    counter = LinearHistogram::FactoryGet(**full_name,
> +    counter = LinearHistogram::FactoryGet(**name,
>                                           min,
>                                           max,
>                                           buckets,
>                                           Histogram::**
> kUmaTargetedHistogramFlag);
>   } else {
> -    counter = Histogram::FactoryGet(full_**name,
> +    counter = Histogram::FactoryGet(name,
>                                     min,
>                                     max,
>                                     buckets,
>
>
>

[Vladislav Kaznacheev, 2011-11-15 14:06:25]

You are right, I forgot about the possibility of overwriting Chrome metrics. 

I think this API should move out of experimental eventually, but I am not sure
whether it should become public or private. The problem with public is that at
the moment there is no way for a 3rd party developer to access the metrics data.
So my vote is for private.

I uploaded a patch that omits the name for component extensions only. This would
solve my original problem (collecting metrics for ChromeOS File Browser).


On 2011/11/15 12:52:29, Roger Tawa wrote:
> The extension id suffix is a security/usability feature to prevent one
> extension from overwriting metrics of chrome itself or other extensions
> that also use the metrics API. The suffix cannot be removed unless there is
> a solution to this. Exceptions could be made for internal extensions.  Are
> you planning to remove this from experimental?
>  On Nov 15, 2011 6:35 AM, <mailto:kaznacheev@chromium.org> wrote:
> 
> > Reviewers: Roger Tawa, hansl,
> >
> > Description:
> > Removing extension name suffix from the metric name in
> > chrome.experimental.metrics
> >
> > Also removed an obsolete comment.
> >
> > Rationale:
> > 1. Extension name postfixes make action names less readable than they need
> > to
> > be.
> > 2. There are not so many extensions using the API. To be precise, at the
> > moment
> > there is exactly one (Chrome OS built-in File Browser).
> > 3. Every new metric needs to be registered in a dashboard configuration
> > file
> > anyway. Names are sorted alphabetically there and duplicates can be easily
> > detected.
> >
> > At the very least we could omit the extension name postfix only for
> > built-in
> > extensions.
> >
> > BUG=
> > TEST=
> >
> >
> > Please review this at
>
http://codereview.chromium.**org/8510046/%3Chttp://codereview.chromium.org/85...>
> >
> > SVN Base:
>
http://git.chromium.org/git/**chromium.git%40trunk%3Chttp://git.chromium.org/...>
> >
> > Affected files:
> >  M chrome/browser/extensions/**extension_metrics_module.cc
> >
> >
> > Index: chrome/browser/extensions/**extension_metrics_module.cc
> > diff --git a/chrome/browser/extensions/**extension_metrics_module.cc
> > b/chrome/browser/extensions/**extension_metrics_module.cc
> > index 8e3428d1e146b8ba8f8d688a4ca313**fff45fad49..**
> > 84d81160a4eaaac4c0e1275837a43f**9f56ede0cb 100644
> > --- a/chrome/browser/extensions/**extension_metrics_module.cc
> > +++ b/chrome/browser/extensions/**extension_metrics_module.cc
> > @@ -14,25 +14,6 @@
> >  using base::Histogram;
> >  using base::LinearHistogram;
> >
> > -namespace {
> > -
> > -// Build the full name of a metrics for the given extension.  Each metric
> > -// is made up of the unique name within the extension followed by the
> > -// extension's id.  This keeps the metrics from one extension unique from
> > -// other extensions, as well as those metrics from chrome itself.
> > -std::string BuildMetricName(const std::string& name,
> > -                            const Extension* extension) {
> > -  std::string full_name(name);
> > -  full_name += extension->id();
> > -  return full_name;
> > -}
> > -
> > -}  // anonymous namespace
> > -
> > -// These extension function classes are enabled only if the
> > -// enable-metrics-extension-api command line switch is used.  Refer to
> > -// extension_function_dispatcher.**cc to see how they are enabled.
> > -
> >  bool MetricsSetEnabledFunction::**RunImpl() {
> >   bool enabled = false;
> >   EXTENSION_FUNCTION_VALIDATE(**args_->GetBoolean(0, &enabled));
> > @@ -56,7 +37,6 @@ bool MetricsRecordUserActionFunctio**n::RunImpl() {
> >   std::string name;
> >   EXTENSION_FUNCTION_VALIDATE(**args_->GetString(0, &name));
> >
> > -  name = BuildMetricName(name, GetExtension());
> >   UserMetrics::**RecordComputedAction(name);
> >   return true;
> >  }
> > @@ -74,16 +54,15 @@ bool MetricsHistogramHelperFunction**::RecordValue(const
> > std::string& name,
> >                                                  int max,
> >                                                  size_t buckets,
> >                                                  int sample) {
> > -  std::string full_name = BuildMetricName(name, GetExtension());
> >   Histogram* counter;
> >   if (type == Histogram::LINEAR_HISTOGRAM) {
> > -    counter = LinearHistogram::FactoryGet(**full_name,
> > +    counter = LinearHistogram::FactoryGet(**name,
> >                                           min,
> >                                           max,
> >                                           buckets,
> >                                           Histogram::**
> > kUmaTargetedHistogramFlag);
> >   } else {
> > -    counter = Histogram::FactoryGet(full_**name,
> > +    counter = Histogram::FactoryGet(name,
> >                                     min,
> >                                     max,
> >                                     buckets,
> >
> >
> >

[jar (doing other things), 2011-11-15 17:25:21]

IMO, such an API should be private.  If you make it public, then it is easy
for any web page to crash the browser by creating an unbounded set of new
histograms... each of which has a memory footprint in the browser...
quickly leading to memory exhaustion.

As noted, non-google developers can't even see these histograms.  You
might, in your interface layer, insist that only a certain extension can
provide input to a specific set of human readaby named hisotgrams, but
adding the extension into the name of each histogram doesn't (at first
blush) seem that helpful.

What am I missing?

Thanks,

Jim

On Tue, Nov 15, 2011 at 6:06 AM, <kaznacheev@chromium.org> wrote:

> You are right, I forgot about the possibility of overwriting Chrome
> metrics.
>
> I think this API should move out of experimental eventually, but I am not
> sure
> whether it should become public or private. The problem with public is
> that at
> the moment there is no way for a 3rd party developer to access the metrics
> data.
> So my vote is for private.
>
> I uploaded a patch that omits the name for component extensions only. This
> would
> solve my original problem (collecting metrics for ChromeOS File Browser).
>
>
>
> On 2011/11/15 12:52:29, Roger Tawa wrote:
>
>> The extension id suffix is a security/usability feature to prevent one
>> extension from overwriting metrics of chrome itself or other extensions
>> that also use the metrics API. The suffix cannot be removed unless there
>> is
>> a solution to this. Exceptions could be made for internal extensions.  Are
>> you planning to remove this from experimental?
>>  On Nov 15, 2011 6:35 AM,
<mailto:kaznacheev@chromium.**org<kaznacheev@chromium.org>>
>> wrote:
>>
>
>  > Reviewers: Roger Tawa, hansl,
>> >
>> > Description:
>> > Removing extension name suffix from the metric name in
>> > chrome.experimental.metrics
>> >
>> > Also removed an obsolete comment.
>> >
>> > Rationale:
>> > 1. Extension name postfixes make action names less readable than they
>> need
>> > to
>> > be.
>> > 2. There are not so many extensions using the API. To be precise, at the
>> > moment
>> > there is exactly one (Chrome OS built-in File Browser).
>> > 3. Every new metric needs to be registered in a dashboard configuration
>> > file
>> > anyway. Names are sorted alphabetically there and duplicates can be
>> easily
>> > detected.
>> >
>> > At the very least we could omit the extension name postfix only for
>> > built-in
>> > extensions.
>> >
>> > BUG=
>> > TEST=
>> >
>> >
>> > Please review this at
>>
>
> http://codereview.chromium.****org/8510046/%3Chttp://coderevi**
> ew.chromium.org/8510046/ <http://codereview.chromium.org/8510046/>>
>
>> >
>> > SVN Base:
>>
>
> http://git.chromium.org/git/****chromium.git%40trunk%3Chttp://**
>
git.chromium.org/git/chromium.**git%40trunk<http://git.chromium.org/git/**chromium.git%40trunk%3Chttp://git.chromium.org/git/chromium.git%40trunk>
> >
>
>> >
>> > Affected files:
>> >  M chrome/browser/extensions/****extension_metrics_module.cc
>> >
>> >
>> > Index: chrome/browser/extensions/****extension_metrics_module.cc
>> > diff --git a/chrome/browser/extensions/****extension_metrics_module.cc
>> > b/chrome/browser/extensions/****extension_metrics_module.cc
>> > index 8e3428d1e146b8ba8f8d688a4ca313****fff45fad49..**
>> > 84d81160a4eaaac4c0e1275837a43f****9f56ede0cb 100644
>> > --- a/chrome/browser/extensions/****extension_metrics_module.cc
>> > +++ b/chrome/browser/extensions/****extension_metrics_module.cc
>>
>> > @@ -14,25 +14,6 @@
>> >  using base::Histogram;
>> >  using base::LinearHistogram;
>> >
>> > -namespace {
>> > -
>> > -// Build the full name of a metrics for the given extension.  Each
>> metric
>> > -// is made up of the unique name within the extension followed by the
>> > -// extension's id.  This keeps the metrics from one extension unique
>> from
>> > -// other extensions, as well as those metrics from chrome itself.
>> > -std::string BuildMetricName(const std::string& name,
>> > -                            const Extension* extension) {
>> > -  std::string full_name(name);
>> > -  full_name += extension->id();
>> > -  return full_name;
>> > -}
>> > -
>> > -}  // anonymous namespace
>> > -
>> > -// These extension function classes are enabled only if the
>> > -// enable-metrics-extension-api command line switch is used.  Refer to
>> > -// extension_function_dispatcher.****cc to see how they are enabled.
>> > -
>> >  bool MetricsSetEnabledFunction::****RunImpl() {
>> >   bool enabled = false;
>> >   EXTENSION_FUNCTION_VALIDATE(****args_->GetBoolean(0, &enabled));
>> > @@ -56,7 +37,6 @@ bool MetricsRecordUserActionFunctio****n::RunImpl() {
>> >   std::string name;
>> >   EXTENSION_FUNCTION_VALIDATE(****args_->GetString(0, &name));
>>
>> >
>> > -  name = BuildMetricName(name, GetExtension());
>> >   UserMetrics::****RecordComputedAction(name);
>> >   return true;
>> >  }
>> > @@ -74,16 +54,15 @@ bool MetricsHistogramHelperFunction**
>> **::RecordValue(const
>>
>> > std::string& name,
>> >                                                  int max,
>> >                                                  size_t buckets,
>> >                                                  int sample) {
>> > -  std::string full_name = BuildMetricName(name, GetExtension());
>> >   Histogram* counter;
>> >   if (type == Histogram::LINEAR_HISTOGRAM) {
>> > -    counter = LinearHistogram::FactoryGet(****full_name,
>> > +    counter = LinearHistogram::FactoryGet(****name,
>> >                                           min,
>> >                                           max,
>> >                                           buckets,
>> >                                           Histogram::**
>> > kUmaTargetedHistogramFlag);
>> >   } else {
>> > -    counter = Histogram::FactoryGet(full_****name,
>>
>> > +    counter = Histogram::FactoryGet(name,
>> >                                     min,
>> >                                     max,
>> >                                     buckets,
>> >
>> >
>> >
>>
>
>
>
>
http://codereview.chromium.**org/8510046/<http://codereview.chromium.org/8510...
>

[Roger Tawa OOO till Jul 10th, 2011-11-15 18:03:24]

lgtm

What does a private api mean exactly?  That only certain extensions can use it,
or that its simply not documented, but still callable by any extension?

One solution to keep the memory from exploding would be to limit the unique
number of metrics any given extension can generate.  I wrote a design proposal
for this api before I first implemented it, see
http://www.chromium.org/developers/design-documents/extensions/metrics-api  This
talks about how the api could be abused and ways to mitigate that abuse.  These
abuses would need to be addressed before making this api non-experimental.

[commit-bot: I haz the power, 2011-11-16 10:29:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaznacheev@chromium.org/8510046/6001

[Vladislav Kaznacheev, 2011-11-16 10:32:27]

Thanks for the review.

I am committing this (with the obsolete comment removed). I will investigate the
best path out of experimental and get back to everyone.

On 2011/11/15 18:03:24, Roger Tawa wrote:
> lgtm
> 
> What does a private api mean exactly?  That only certain extensions can use
it,
> or that its simply not documented, but still callable by any extension?
> 
> One solution to keep the memory from exploding would be to limit the unique
> number of metrics any given extension can generate.  I wrote a design proposal
> for this api before I first implemented it, see
> http://www.chromium.org/developers/design-documents/extensions/metrics-api 
This
> talks about how the api could be abused and ways to mitigate that abuse. 
These
> abuses would need to be addressed before making this api non-experimental.

[commit-bot: I haz the power, 2011-11-16 13:07:13]

Change committed as 110295

[Ilya Sherman, 2011-11-16 21:25:01]

On 2011/11/15 18:03:24, Roger Tawa wrote:
> lgtm
> 
> What does a private api mean exactly?  That only certain extensions can use
it,
> or that its simply not documented, but still callable by any extension?

In this case, I would expect the metrics logging methods to only do something
for whitelisted extensions, whose authors have access to the collected metrics
data.  It sounds like currently, that whitelist could be defined simply as
"component extensions".  Does that sound reasonable?