Make it harder to get ContentSettingsType histogram values wrong

Make it harder to get ContentSettingsType histogram values wrong

This CL forces the histogram value for a content setting to be
explicitly specified to reduce the risk of getting out of sync with
histograms.xml.

BUG=697234
Review-Url: https://codereview.chromium.org/2728303002
Cr-Commit-Position: refs/heads/master@{#465076}
Committed: https://chromium.googlesource.com/chromium/src/+/c5e0038e479176c8e130882203ca4567f169b38b

[raymes, 2017-03-06 01:19:43]

raymes@chromium.org changed reviewers:
+ lgarron@chromium.org

[raymes, 2017-03-06 01:55:30]

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

[commit-bot: I haz the power, 2017-03-06 01:55:47]

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-03-06 03:08:52]

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

[commit-bot: I haz the power, 2017-03-06 03:08:53]

Dry run: This issue passed the CQ dry run.

[raymes, 2017-03-13 02:25:12]

raymes@chromium.org changed reviewers:
+ msramek@chromium.org
- lgarron@chromium.org

[raymes, 2017-03-13 02:25:13]

msramek: could you ptal? Thanks!

[msramek, 2017-03-13 10:17:23]

So I can finally close https://codereview.chromium.org/2129153002/ which I was
totally going to address one day? :-)

LGTM, thanks for cleaning this up!

[raymes, 2017-03-15 01:47:37]

The CQ bit was checked by raymes@chromium.org

[commit-bot: I haz the power, 2017-03-15 01:48:26]

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-03-15 01:59:32]

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

[commit-bot: I haz the power, 2017-03-15 01:59:33]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[raymes.khoury, 2017-03-16 00:19:32]

raymes.khoury@gmail.com changed reviewers:
+ mpearson@chromium.org, raymes.khoury@gmail.com

[raymes.khoury, 2017-03-16 00:19:32]

+mpearson for histograms

[raymes, 2017-03-16 21:40:01]

raymes@chromium.org changed reviewers:
+ isherman@chromium.org
- raymes.khoury@gmail.com

[raymes, 2017-03-16 21:40:02]

+isherman for histograms

[Ilya Sherman, 2017-03-16 22:19:41]

LGTM with a request:

https://codereview.chromium.org/2728303002/diff/1/components/content_settings...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/2728303002/diff/1/components/content_settings...
components/content_settings/core/common/content_settings.cc:25: // specified in
the ContentType enum in histograms.xml.
Please preserve the previous WARNING, or something akin to it -- it's still
important to never reuse identifiers for multiple semantic meanings.

[Mark P, 2017-03-16 22:31:45]

I didn't look in detail, but the linked bug implies that some clients added
enums out of order.  Isn't this one of those times when it's appropriate to
rename the old histogram to get a clean break from earlier data with different
semantics (different chrome version mean different things for the same value,
say, 27).

--mark

[raymes, 2017-03-17 07:44:51]

On 2017/03/16 22:31:45, Mark P wrote:
> I didn't look in detail, but the linked bug implies that some clients added
> enums out of order.  Isn't this one of those times when it's appropriate to
> rename the old histogram to get a clean break from earlier data with different
> semantics (different chrome version mean different things for the same value,
> say, 27).
> 
> --mark

I can change it such that the actual values themselves reported by Chrome stay
the same but the histograms.xml labels are corrected for that value. Would that
be better? 

There are 10 histograms that depend on this enum so I'd like to avoid adding all
new ones if at all possible :) It's only 1 label that is out of whack.

[Mark P, 2017-03-17 18:03:09]

On Fri, Mar 17, 2017 at 12:44 AM, <raymes@chromium.org> wrote:

> On 2017/03/16 22:31:45, Mark P wrote:
> > I didn't look in detail, but the linked bug implies that some clients
> added
> > enums out of order. Isn't this one of those times when it's appropriate
> to
> > rename the old histogram to get a clean break from earlier data with
> different
> > semantics (different chrome version mean different things for the same
> value,
> > say, 27).
> >
> > --mark
>
> I can change it such that the actual values themselves reported by Chrome
> stay
> the same
>

I don't think that's useful.

This histogram appears to already be messed up.
At some point in the past, there was this code in the middle of the
histogram
#if defined(OS_ANDROID) || defined(OS_CHROMEOS)
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER,
#endif

This means that on some platforms value 21 means
CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER and on other platforms it
means CONTENT_SETTINGS_TYPE_APP_BANNER.

Furthermore, at some point
https://codereview.chromium.org/1706503002/diff/280001/components/content_set...
was submitted, which put a new enum not at the end.  This means that in
some builds of the browser the value that was CONTENT_SETTINGS_TYPE_KEYGEN
is now CONTENT_SETTINGS_TYPE_BLUETOOTH_GUARD.

I don't see a way to work around this.  You'd need everyone looking at one
of these histograms to remember to slice by platform (because the
interpretation of a bucket values by platform) and to slice by version
(because the bucket meaning varies by version) and then read different
numeric values as different enums under those conditions.  And trust
everyone to remember to do this every time for every one of those
histograms without making a mistake.

+isherman,
It sounds like this has gotten messed up enough, repeatedly, that we should
put this file under narrower OWNERS review like we do with UseCounter.h.
What do you think?

--mark



> but the histograms.xml labels are corrected for that value. Would that
> be better?
>
> There are 10 histograms that depend on this enum so I'd like to avoid
> adding all
> new ones if at all possible :) It's only 1 label that is out of whack.
>
> https://codereview.chromium.org/2728303002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2017-03-17 19:42:45]

On 2017/03/17 18:03:09, Mark P wrote:
> On Fri, Mar 17, 2017 at 12:44 AM, <mailto:raymes@chromium.org> wrote:
> 
> > On 2017/03/16 22:31:45, Mark P wrote:
> > > I didn't look in detail, but the linked bug implies that some clients
> > added
> > > enums out of order. Isn't this one of those times when it's appropriate
> > to
> > > rename the old histogram to get a clean break from earlier data with
> > different
> > > semantics (different chrome version mean different things for the same
> > value,
> > > say, 27).
> > >
> > > --mark
> >
> > I can change it such that the actual values themselves reported by Chrome
> > stay
> > the same
> >
> 
> I don't think that's useful.
> 
> This histogram appears to already be messed up.
> At some point in the past, there was this code in the middle of the
> histogram
> #if defined(OS_ANDROID) || defined(OS_CHROMEOS)
> CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER,
> #endif
> 
> This means that on some platforms value 21 means
> CONTENT_SETTINGS_TYPE_PROTECTED_MEDIA_IDENTIFIER and on other platforms it
> means CONTENT_SETTINGS_TYPE_APP_BANNER.
> 
> Furthermore, at some point
>
https://codereview.chromium.org/1706503002/diff/280001/components/content_set...
> was submitted, which put a new enum not at the end.  This means that in
> some builds of the browser the value that was CONTENT_SETTINGS_TYPE_KEYGEN
> is now CONTENT_SETTINGS_TYPE_BLUETOOTH_GUARD.
> 
> I don't see a way to work around this.  You'd need everyone looking at one
> of these histograms to remember to slice by platform (because the
> interpretation of a bucket values by platform) and to slice by version
> (because the bucket meaning varies by version) and then read different
> numeric values as different enums under those conditions.  And trust
> everyone to remember to do this every time for every one of those
> histograms without making a mistake.

Thanks for digging into the history here, Mark!  I hadn't realized it had gotten
so out of whack.  I agree that it's probably better to rename those ten
histograms.

> +isherman,
> It sounds like this has gotten messed up enough, repeatedly, that we should
> put this file under narrower OWNERS review like we do with UseCounter.h.
> What do you think?

I'd defer to the team that owns these metrics, what they want to do.  I think
it's plausible that the current CL will tend to prevent most errors from
continuing to crop up here.  Perhaps there could be a light presubmit check that
verifies that histograms.xml is updated whenever this enum is updated?

[Ilya Sherman, 2017-03-17 22:59:30]

(To be explicit, not lgtm while we're discussing how best to handle the
previously jumbled semantics.)

[raymes, 2017-03-18 06:21:36]

I'm not sure I agree that things are so out of whack. The histogram isn't
based on the ContentSettingsType enum. Each enum value corresponds to a
different value which is what has been recorded in the histogram. This
conversion is done in ContentSettingTypeToHistogramValue in
content_settings.cc. In my looking through it, only that one type was
wrong...

On Sat, 18 Mar 2017 at 09:59 <isherman@chromium.org> wrote:

> (To be explicit, not lgtm while we're discussing how best to handle the
> previously jumbled semantics.)
>
> https://codereview.chromium.org/2728303002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2017-03-21 22:30:45]

On 2017/03/18 06:21:36, raymes (OOO until April 10) wrote:
> I'm not sure I agree that things are so out of whack. The histogram isn't
> based on the ContentSettingsType enum. Each enum value corresponds to a
> different value which is what has been recorded in the histogram. This
> conversion is done in ContentSettingTypeToHistogramValue in
> content_settings.cc. In my looking through it, only that one type was
> wrong...

Sorry, I'm not following your reasoning.  It looks like at a minimum,
CONTENT_SETTINGS_TYPE_MEDIASTREAM was removed from the enum without any
replacement for a long while, which shifted all of the identifiers for every
type listed below it.  Am I misunderstanding something there?  Which type is the
single type that you think was wrong?

[raymes, 2017-04-11 01:25:16]

My apologies for the delay on this - I was unexpectedly OOO for a couple of
weeks!

This is my understanding of things:
-There was an error introduced where MEDIASTREAM was removed for a month or
so and then fixed. This put a months worth of data out of whack, but that
was about a year ago now. I don't think it will impact analysis we do now
significantly.
-Besides that the only error I am aware of is that BLUETOOTH_GUARD is
currently being labelled (in histograms.xml) as "Background sync setting".
My understanding is that changing the histograms.xml label should correct
that error without any problems when analysing historical data.

Given the above, my perspective is that it's not worth the churn of
deprecating all the old histograms that rely on this and adding new ones.
This CL corrects the second error above and also makes it harder to
introduce errors in the future.

What are your thoughts? Thanks!

On Wed, 22 Mar 2017 at 09:30 <isherman@chromium.org> wrote:

On 2017/03/18 06:21:36, raymes (OOO until April 10) wrote:
> I'm not sure I agree that things are so out of whack. The histogram isn't
> based on the ContentSettingsType enum. Each enum value corresponds to a
> different value which is what has been recorded in the histogram. This
> conversion is done in ContentSettingTypeToHistogramValue in
> content_settings.cc. In my looking through it, only that one type was
> wrong...

Sorry, I'm not following your reasoning. It looks like at a minimum,
CONTENT_SETTINGS_TYPE_MEDIASTREAM was removed from the enum without any
replacement for a long while, which shifted all of the identifiers for every
type listed below it. Am I misunderstanding something there? Which type is
the
single type that you think was wrong?

https://codereview.chromium.org/2728303002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2017-04-11 01:31:42]

On 2017/04/11 01:25:16, raymes wrote:
> My apologies for the delay on this - I was unexpectedly OOO for a couple of
> weeks!
> 
> This is my understanding of things:
> -There was an error introduced where MEDIASTREAM was removed for a month or
> so and then fixed. This put a months worth of data out of whack, but that
> was about a year ago now. I don't think it will impact analysis we do now
> significantly.
> -Besides that the only error I am aware of is that BLUETOOTH_GUARD is
> currently being labelled (in histograms.xml) as "Background sync setting".
> My understanding is that changing the histograms.xml label should correct
> that error without any problems when analysing historical data.
> 
> Given the above, my perspective is that it's not worth the churn of
> deprecating all the old histograms that rely on this and adding new ones.
> This CL corrects the second error above and also makes it harder to
> introduce errors in the future.
> 
> What are your thoughts? Thanks!

If the only non-correctable data error happened over a year ago, then how about
this middle-ground suggestion: You could add a note to each of the affected
histograms, instructing users not to trust data before the affected date.  WDYT?
 (Mark, also, are you comfortable with this middle-ground solution?)

[raymes, 2017-04-11 01:52:12]

On 2017/04/11 01:31:42, Ilya Sherman wrote:
> On 2017/04/11 01:25:16, raymes wrote:
> > My apologies for the delay on this - I was unexpectedly OOO for a couple of
> > weeks!
> > 
> > This is my understanding of things:
> > -There was an error introduced where MEDIASTREAM was removed for a month or
> > so and then fixed. This put a months worth of data out of whack, but that
> > was about a year ago now. I don't think it will impact analysis we do now
> > significantly.
> > -Besides that the only error I am aware of is that BLUETOOTH_GUARD is
> > currently being labelled (in histograms.xml) as "Background sync setting".
> > My understanding is that changing the histograms.xml label should correct
> > that error without any problems when analysing historical data.
> > 
> > Given the above, my perspective is that it's not worth the churn of
> > deprecating all the old histograms that rely on this and adding new ones.
> > This CL corrects the second error above and also makes it harder to
> > introduce errors in the future.
> > 
> > What are your thoughts? Thanks!
> 
> If the only non-correctable data error happened over a year ago, then how
about
> this middle-ground suggestion: You could add a note to each of the affected
> histograms, instructing users not to trust data before the affected date. 
WDYT?
>  (Mark, also, are you comfortable with this middle-ground solution?)

That sounds great to me if you folks are ok with it :)

[Jialiu Lin, 2017-04-12 01:03:50]

jialiul@chromium.org changed reviewers:
+ jialiul@chromium.org

[Jialiu Lin, 2017-04-12 01:03:51]

https://codereview.chromium.org/2728303002/diff/40001/components/content_sett...
File components/content_settings/core/common/content_settings.cc (right):

https://codereview.chromium.org/2728303002/diff/40001/components/content_sett...
components/content_settings/core/common/content_settings.cc:58:
{CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER, 31},
Do you mind adding  "CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION" as 32 here and
in the histograms.xml?

[raymes, 2017-04-12 03:11:48]

On 2017/04/12 01:03:51, Jialiu Lin wrote:
>
https://codereview.chromium.org/2728303002/diff/40001/components/content_sett...
> File components/content_settings/core/common/content_settings.cc (right):
> 
>
https://codereview.chromium.org/2728303002/diff/40001/components/content_sett...
> components/content_settings/core/common/content_settings.cc:58:
> {CONTENT_SETTINGS_TYPE_SUBRESOURCE_FILTER, 31},
> Do you mind adding  "CONTENT_SETTINGS_TYPE_PASSWORD_PROTECTION" as 32 here and
> in the histograms.xml?

Thanks Jialiu, I will add the new items in a followup CL if this CL gets
approved.

[Mark P, 2017-04-12 03:57:52]

On 2017/04/11 01:31:42, Ilya Sherman wrote:
> On 2017/04/11 01:25:16, raymes wrote:
> > My apologies for the delay on this - I was unexpectedly OOO for a couple of
> > weeks!
> > 
> > This is my understanding of things:
> > -There was an error introduced where MEDIASTREAM was removed for a month or
> > so and then fixed. This put a months worth of data out of whack, but that
> > was about a year ago now. I don't think it will impact analysis we do now
> > significantly.
> > -Besides that the only error I am aware of is that BLUETOOTH_GUARD is
> > currently being labelled (in histograms.xml) as "Background sync setting".
> > My understanding is that changing the histograms.xml label should correct
> > that error without any problems when analysing historical data.
> > 
> > Given the above, my perspective is that it's not worth the churn of
> > deprecating all the old histograms that rely on this and adding new ones.
> > This CL corrects the second error above and also makes it harder to
> > introduce errors in the future.
> > 
> > What are your thoughts? Thanks!
> 
> If the only non-correctable data error happened over a year ago, then how
about
> this middle-ground suggestion: You could add a note to each of the affected
> histograms, instructing users not to trust data before the affected date. 
WDYT?
>  (Mark, also, are you comfortable with this middle-ground solution?)

I am comfortable with this middle-ground solution.  A good warning / note I
believe
can obviate the need for a histogram rename.

--mark

[raymes, 2017-04-13 03:45:34]

Thanks all for the feedback, I've updated the CL. PTAL when you get a chance :)

[raymes, 2017-04-13 03:45:39]

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

[commit-bot: I haz the power, 2017-04-13 03:46:10]

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-13 05:06:13]

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

[commit-bot: I haz the power, 2017-04-13 05:06:14]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-04-13 07:41:49]

Metrics LGTM, thanks.

[Mark P, 2017-04-13 22:09:08]

Description was changed from

==========
Make it harder to get ContentSettingsType histogram values wrong

This CL forces the histogram value for a content setting to be
explicitly specified to reduce the risk of getting out of sync with
histograms.xml.

BUG=697234
==========

to

==========
Make it harder to get ContentSettingsType histogram values wrong

This CL forces the histogram value for a content setting to be
explicitly specified to reduce the risk of getting out of sync with
histograms.xml.

BUG=697234
==========

[Mark P, 2017-04-13 22:09:08]

mpearson@chromium.org changed reviewers:
- mpearson@chromium.org

[Jialiu Lin, 2017-04-14 22:02:29]

LGTM.  Thanks raymes!

[raymes, 2017-04-17 23:14:52]

The CQ bit was checked by raymes@chromium.org

[raymes, 2017-04-17 23:14:53]

The patchset sent to the CQ was uploaded after l-g-t-m from msramek@chromium.org
Link to the patchset: https://codereview.chromium.org/2728303002/#ps60001
(title: "Make it harder to get ContentSettingsType histogram values wrong")

[commit-bot: I haz the power, 2017-04-17 23:15:09]

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-18 00:27:50]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492470892339890,
"parent_rev": "daaea2c2c745c2f0983de26d7b812d34622773b4", "commit_rev":
"c5e0038e479176c8e130882203ca4567f169b38b"}

[commit-bot: I haz the power, 2017-04-18 00:28:39]

Description was changed from

==========
Make it harder to get ContentSettingsType histogram values wrong

This CL forces the histogram value for a content setting to be
explicitly specified to reduce the risk of getting out of sync with
histograms.xml.

BUG=697234
==========

to

==========
Make it harder to get ContentSettingsType histogram values wrong

This CL forces the histogram value for a content setting to be
explicitly specified to reduce the risk of getting out of sync with
histograms.xml.

BUG=697234

Review-Url: https://codereview.chromium.org/2728303002
Cr-Commit-Position: refs/heads/master@{#465076}
Committed:
https://chromium.googlesource.com/chromium/src/+/c5e0038e479176c8e130882203ca...
==========

[commit-bot: I haz the power, 2017-04-18 00:28:40]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/c5e0038e479176c8e130882203ca...