1343855 - Allow restricting histogram keys to a known set in Histograms.json

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

We have use-cases going around that benefit from using keyed histograms, but actually do know the set of keys in advance (e.g. bug 1254099).

We should:
- for Histograms.json
  - add an optional property 'keys'
  - only available for keyed histograms
  - value is a list of strings, specifying the valid keys for the histogram
- in the Telemetry implementation
  - have a list of valid keys per histogram
  - ignore adding to a histogram with an unspecified key
  - but print a warning

Comment 1

[Chris H-C :chutten]

Instead of (or in addition to) printing a warning, should we accumulate to an N+1th "OTHER" bucket?

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Comment 3

[Georg Fritzsche [:gfritzsche]]

Mauro, this adds a new field 'keys' to Histograms.json entries.
I assume i have to get a PR - with the new histogram_tools.py version - merged into python_moztelemetry before this can land?

Are there any tests i should run on this?
I tested this locally with an old Histograms.json from last year and made that pass.

Comment 4

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten|pto until Mar 20 from comment #1)
> Instead of (or in addition to) printing a warning, should we accumulate to
> an N+1th "OTHER" bucket?

Good question, is there value in transmitting that data for all histograms?
Will that actually get used?

I'm leaning towards client-side logging & testing usually being sufficient.

Comment 5

[Alessio Placitelli [:Dexter]]

Comment on attachment 8846073 [details] [diff] [review]
Part 1 - Support 'keys' property for Histograms.json

Review of attachment 8846073 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good with the linting fixes below.

::: toolkit/components/telemetry/histogram_tools.py
@@ +309,4 @@
>          if len(invalid) > 0:
>              raise ValueError, 'Label values for %s are not matching pattern "%s": %s' % \
> +                              (name, CPP_IDENTIFIER_PATTERN, ', '.join(invalid))
> +

nit: please add a new blank before and after check_keys_field, so that there are two blank lines between the functions. Otherwise linting will fail.

@@ +315,5 @@
> +        if not self._strict_type_checks or not keys:
> +            return
> +
> +        if keys and not definition.get('keyed', False):
> +            raise ValueError, "'keys' field is not valid for %s; only allowed for keyed histograms." % (name)

We're forbidding this way of rising exceptions in bug 1344852. Since this patch is probably landing after that one, you could probably just use the new style. So this exception (and the ones below) would become:

raise ValueError("'keys' field is not valid for %s; only allowed for keyed histograms." % (name))

Comment 6

[Mathieu Leplatre [:leplatrem]]

After further discussions with :mostlygeek yesterday, we realized that restricting the keys might not be a requirement for our use-case.

Indeed, it came to our mind that leaving the list of keys open allows us to have a generic solution that could monitor any kind of remote update. Including unforeseen remote data updates that could be introduced out of release trains — like via system-addons.

Georg, I remember that you suggested this while looking at our use-case ([1]). Do you have any strong reason to suggest limiting the keys (appart from cleanliness/integrity)?

In case we drop this, I hope I don't come up too late with this feedback :(

Thanks for your feedback!

[1] https://docs.google.com/document/d/1TocR6EzNNWynvuYNgxWZZfV57QRITSfoDC9p7onnlg4/edit#

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to Mathieu Leplatre (:leplatrem) from comment #6)
> After further discussions with :mostlygeek yesterday, we realized that
> restricting the keys might not be a requirement for our use-case.
> 
> Indeed, it came to our mind that leaving the list of keys open allows us to
> have a generic solution that could monitor any kind of remote update.
> Including unforeseen remote data updates that could be introduced out of
> release trains — like via system-addons.
> 
> Georg, I remember that you suggested this while looking at our use-case
> ([1]). Do you have any strong reason to suggest limiting the keys (appart
> from cleanliness/integrity)?

I see, using an unrestricted keyed histogram makes sense with that additional information.

This bug is useful anyway independent of bug 1254099, so lets take further discussion to that bug.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> Mauro, this adds a new field 'keys' to Histograms.json entries.
> I assume i have to get a PR - with the new histogram_tools.py version -
> merged into python_moztelemetry before this can land?
> 
> Are there any tests i should run on this?
> I tested this locally with an old Histograms.json from last year and made
> that pass.

Roberto, do you know?

Comment 9

[Roberto Agostino Vitillo (:rvitillo)]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #3)
> > Mauro, this adds a new field 'keys' to Histograms.json entries.
> > I assume i have to get a PR - with the new histogram_tools.py version -
> > merged into python_moztelemetry before this can land?

It is really unfortunate that we still don't have this file in a versioned python package. 

It's fine to merge this in moztelemetry after it has landed, as long as the changes don't change the existing API. Please file a PR to do so once this has landed.

> > Are there any tests i should run on this?
> > I tested this locally with an old Histograms.json from last year and made
> > that pass.

The tests you should run are the ones part of the moztelemetry library.

Comment 10

[Alessio Placitelli [:Dexter]]

Does this need to be tested in moztelemetry and landed in mozilla central?

Or does it need some changes too?

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #10)
> Does this need to be tested in moztelemetry and landed in mozilla central?
> 
> Or does it need some changes too?

We need this to also land in moztelemetry, AFAICT moztelemetry jobs will start to fail otherwise once we add the new property.

We could also consider to change the script to ignore any unknown properties on the server-side.

Comment 12

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> (In reply to Alessio Placitelli [:Dexter] from comment #10)
> > Does this need to be tested in moztelemetry and landed in mozilla central?
> > 
> > Or does it need some changes too?
> 
> We need this to also land in moztelemetry, AFAICT moztelemetry jobs will
> start to fail otherwise once we add the new property.

I've filed a PR (https://github.com/mozilla/python_moztelemetry/pull/148) which is automatically running the tests. Let's see how the new file behaves there.

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Rebased, carrying over r+.

Comment 14

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

I was carrying over the r+, but I made a slight change to this. Would you mind reviewing it? The difference being exposing the allowed keys through the |def keys| function.

Comment 15

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

This patch adds the support for "key whitelists" in keyed histograms in the telemetry core:

- Changes TelemetryHistogram.cpp to expose the key whitelist and adds the key check on accumulation;
- Prints a warning and discards the accumulation if the key is not allowed on keyed histograms;
- Updates the docs;
- Adds test coverage.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8864186 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Review of attachment 8864186 [details] [diff] [review]:
-----------------------------------------------------------------

This only adds support for the JS API.
Do you plan to add support for the C++ API?
And will the C++ API get a fast path with enum keys?

Comment 17

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> Comment on attachment 8864186 [details] [diff] [review]
> Part 2 - Support key whitelists in keyed histograms
> 
> Review of attachment 8864186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This only adds support for the JS API.
> Do you plan to add support for the C++ API?

Good point. I forgot about this :-S
I will add support for the C++ API and test coverage.

> And will the C++ API get a fast path with enum keys?

I was not planning to do that.
Do you see this feature being used extensively with pre-defined keys on C++ hotpaths?
Should I look into that or can we defer this?

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #17)
> Should I look into that or can we defer this?

Let's take that to a follow-up bug then.
This would fix performance concerns for using keyed histograms in C++.

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Comment 20

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #18)
> (In reply to Alessio Placitelli [:Dexter] from comment #17)
> > Should I look into that or can we defer this?
> 
> Let's take that to a follow-up bug then.
> This would fix performance concerns for using keyed histograms in C++.

Cool! I've filed bug 1361960 for this.

I've updated the part 2 with the C++ API support. It comes with bonus gtest coverage as well :)

Comment 21

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8864185 [details] [diff] [review]
Part 1 - Support 'keys' property for Histograms.json

Review of attachment 8864185 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I guess this is still r=dexter though.

Comment 22

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8864526 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Review of attachment 8864526 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good. I left mostly smaller comments here, but there are two important points:
- I don't think allowing `[]` in Histograms.json is meaningful. We should require non-empty lists.
- Should we track the discarding in a diagnostic probe?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +308,5 @@
> +  MOZ_ASSERT(key);
> +  MOZ_ASSERT(this->keyed);
> +
> +  // If we didn't specify a list of allowed keys, just return true.
> +  if (!this->key_count) {

`... == 0`

@@ +324,5 @@
> +      return true;
> +    }
> +  }
> +
> +  // It wasn't, so return false.

"return false" seems redundant.
"|key| was not found."?

@@ +2032,5 @@
>    }
>  
> +  // Check if we're allowed to record in the provided key, for this histogram.
> +  if (!gHistograms[aID].allows_key(aKey.get())) {
> +    LogToBrowserConsole(nsIScriptError::errorFlag,

Both this and the function below end up calling `internal_Accumulate()` and the logic is duplicated.
But i see that this ends up requiring passing status codes around.

We should really refactor the status/warning/error flow here, but that can happen around bug 1278821 (as a follow-up or blocker).
Can you file something?

::: toolkit/components/telemetry/docs/collection/histograms.rst
@@ +138,5 @@
>  
> +``keys``
> +---------
> +Optional, list of strings, defaults to ``[]`` (allow all keys). Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. If a key that is
> +not in the list is used, the accumulation is discarded and a warning is printed to the browser console.

I don't think allowing `[]` in Histograms.json is meaningful. We should require non-empty lists.

Nit: "When using a key that is not in the list ..."?

Should we track the discarding in a diagnostic probe?

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +946,5 @@
> +  h.clear();
> +
> +  // The |add| method should not throw for keys that are not allowed.
> +  for (let v of ["testkey", "CommonKey", "not-allowed"]) {
> +    h.add(v, true);

Please test against different values for both allowed keys.
This provides test coverage for e.g. key ordering issues.

@@ +954,5 @@
> +  let snap = h.snapshot();
> +  Assert.ok(Object.keys(snap).length, 2, "Only 2 keys must be recorded.");
> +  for (let k of ["testkey", "CommonKey"]) {
> +    Assert.ok(k in snap, `{k} must be recorded`);
> +    Assert.deepEqual(snap[k].counts, [0, 1, 0], `{k} must contain the correct value.`);

You need to use `${foo}` for template strings.
Also nit: Missing period to finish the sentence.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> @@ +2032,5 @@
> >    }
> >  
> > +  // Check if we're allowed to record in the provided key, for this histogram.
> > +  if (!gHistograms[aID].allows_key(aKey.get())) {
> > +    LogToBrowserConsole(nsIScriptError::errorFlag,
> 
> Both this and the function below end up calling `internal_Accumulate()` and
> the logic is duplicated.
> But i see that this ends up requiring passing status codes around.
> 
> We should really refactor the status/warning/error flow here, but that can
> happen around bug 1278821 (as a follow-up or blocker).
> Can you file something?

Mh, I'm not sure what you mean here :(
Both function call an |internal_Accumulate()|, but one calls the version with no key and the other the version that requires a key.
Should I still file a bug about refactoring this?

> ::: toolkit/components/telemetry/docs/collection/histograms.rst
> @@ +138,5 @@
> >  
> > +``keys``
> > +---------
> > +Optional, list of strings, defaults to ``[]`` (allow all keys). Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. If a key that is
> > +not in the list is used, the accumulation is discarded and a warning is printed to the browser console.
> 
> I don't think allowing `[]` in Histograms.json is meaningful. We should
> require non-empty lists.
> 
> Nit: "When using a key that is not in the list ..."?
> 
> Should we track the discarding in a diagnostic probe?

I guess we could do that and it could be helpful in case some consumer of an histogram using this would suddenly go crazy and start using unknown keys. I think this qualifies be an opt-out, never expiring, keyed (by histogram name) exponential histogram probe or scalar count. Any concern about this?

Comment 24

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #23)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> > @@ +2032,5 @@
> > >    }
> > >  
> > > +  // Check if we're allowed to record in the provided key, for this histogram.
> > > +  if (!gHistograms[aID].allows_key(aKey.get())) {
> > > +    LogToBrowserConsole(nsIScriptError::errorFlag,
> > 
> > Both this and the function below end up calling `internal_Accumulate()` and
> > the logic is duplicated.
> > But i see that this ends up requiring passing status codes around.
> > 
> > We should really refactor the status/warning/error flow here, but that can
> > happen around bug 1278821 (as a follow-up or blocker).
> > Can you file something?
> 
> Mh, I'm not sure what you mean here :(
> Both function call an |internal_Accumulate()|, but one calls the version
> with no key and the other the version that requires a key.
> Should I still file a bug about refactoring this?

Right, i see. The key check should probably still live inside `internal_Accumulate()`.
If we do this now though, then you can't log errors directly.
So you would need to pass status codes out to print them from the non-locked part, similar to how the scalars do 3-phase JS function implementation.
This sounds like a follow-up after bug 1278821.

> > ::: toolkit/components/telemetry/docs/collection/histograms.rst
> > @@ +138,5 @@
> > >  
> > > +``keys``
> > > +---------
> > > +Optional, list of strings, defaults to ``[]`` (allow all keys). Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. If a key that is
> > > +not in the list is used, the accumulation is discarded and a warning is printed to the browser console.
> > 
> > I don't think allowing `[]` in Histograms.json is meaningful. We should
> > require non-empty lists.
> > 
> > Nit: "When using a key that is not in the list ..."?
> > 
> > Should we track the discarding in a diagnostic probe?
> 
> I guess we could do that and it could be helpful in case some consumer of an
> histogram using this would suddenly go crazy and start using unknown keys. I
> think this qualifies be an opt-out, never expiring, keyed (by histogram
> name) exponential histogram probe or scalar count. Any concern about this?

Sounds like a plan. Keyed scalar count sounds sufficient?

Comment 25

[Georg Fritzsche [:gfritzsche]]

On a related note, can you file a follow-up to implement the "known keys" property for scalars as well?

Comment 26

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #24)
> (In reply to Alessio Placitelli [:Dexter] from comment #23)
> > (In reply to Georg Fritzsche [:gfritzsche] from comment #22)
> > > @@ +2032,5 @@
> > > >    }
> > > >  
> > > > +  // Check if we're allowed to record in the provided key, for this histogram.
> > > > +  if (!gHistograms[aID].allows_key(aKey.get())) {
> > > > +    LogToBrowserConsole(nsIScriptError::errorFlag,
> > > 
> > > Both this and the function below end up calling `internal_Accumulate()` and
> > > the logic is duplicated.
> > > But i see that this ends up requiring passing status codes around.
> > > 
> > > We should really refactor the status/warning/error flow here, but that can
> > > happen around bug 1278821 (as a follow-up or blocker).
> > > Can you file something?
> > 
> > Mh, I'm not sure what you mean here :(
> > Both function call an |internal_Accumulate()|, but one calls the version
> > with no key and the other the version that requires a key.
> > Should I still file a bug about refactoring this?
> 
> Right, i see. The key check should probably still live inside
> `internal_Accumulate()`.
> If we do this now though, then you can't log errors directly.
> So you would need to pass status codes out to print them from the non-locked
> part, similar to how the scalars do 3-phase JS function implementation.
> This sounds like a follow-up after bug 1278821.

Filed bug 1365526.

> > > ::: toolkit/components/telemetry/docs/collection/histograms.rst
> > > @@ +138,5 @@
> > > >  
> > > > +``keys``
> > > > +---------
> > > > +Optional, list of strings, defaults to ``[]`` (allow all keys). Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. If a key that is
> > > > +not in the list is used, the accumulation is discarded and a warning is printed to the browser console.
> > > 
> > > I don't think allowing `[]` in Histograms.json is meaningful. We should
> > > require non-empty lists.
> > > 
> > > Nit: "When using a key that is not in the list ..."?
> > > 
> > > Should we track the discarding in a diagnostic probe?
> > 
> > I guess we could do that and it could be helpful in case some consumer of an
> > histogram using this would suddenly go crazy and start using unknown keys. I
> > think this qualifies be an opt-out, never expiring, keyed (by histogram
> > name) exponential histogram probe or scalar count. Any concern about this?
> 
> Sounds like a plan. Keyed scalar count sounds sufficient?

Sure!

(In reply to Georg Fritzsche [:gfritzsche] from comment #25)
> On a related note, can you file a follow-up to implement the "known keys"
> property for scalars as well?

Filed bug 1365529.

Comment 27

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Comment 28

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Thanks for the review Georg, this should deal with all your feedback.

Comment 29

[Alessio Placitelli [:Dexter]]

Comment on attachment 8868462 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Whoops, I forgot to add the scalar :-D

Comment 30

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Added the scalar and extended the JS test coverage. I thought about checking the value of the scalar probe in the gtest code too, but that would have been... messy.

Maybe we should refactor & share all the scalar/histogram/event related helper functions for tests to simplify that in the future.

Comment 31

[Alessio Placitelli [:Dexter]]

Dropping & moving to p2. We'll pick it up once the dependency is gone.

Comment 32

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8868498 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Review of attachment 8868498 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good overall.

In order to land this we first should get at least bug 1367750 landed first.

1: https://github.com/georgf/probe-scraper/commits/fallout-fix

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +20,5 @@
>  #include "mozilla/StartupTimeline.h"
>  #include "mozilla/StaticMutex.h"
>  #include "mozilla/Unused.h"
>  
> +#include "Telemetry.h"

Include TelemetryScalar.h and use TelemetryScalar::*?

@@ +320,5 @@
> +    // gHistogramStringTable. They are stored in-order and consecutively,
> +    // from the offset key_index to (key_index + key_count).
> +    uint32_t string_offset = gHistogramKeyTable[this->key_index + i];
> +    const char* const str = &gHistogramStringTable[string_offset];
> +    if (::strcmp(key, str) == 0) {

Why not make this:
- accept a ns*String
- use *.EqualsASCII(str)

::: toolkit/components/telemetry/docs/collection/histograms.rst
@@ +137,5 @@
>  Optional, boolean, defaults to ``false``. Determines whether this is a *keyed histogram*.
>  
> +``keys``
> +---------
> +Optional, list of strings. Only valid for *keyed histograms*. Defines a case sensitive list of allowed keys that can be used for this histogram. When using a key that is not in the list, the accumulation is discarded and a warning is printed to the browser console.

What is the limit for number of keys?

Comment 33

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Rebased, again

Comment 34

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Comment 35

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8882051 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Review of attachment 8882051 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good with the below questions/comments cleared up.

Please check with Chris if this should wait to land after bug 1366294 (in case it will be effort to merge).

::: toolkit/components/telemetry/Histograms.json
@@ +7514,5 @@
> +    "kind": "boolean",
> +    "keyed": true,
> +    "keys": [
> +      "testkey",
> +      "CommonKey"

For making sure indices are used correctly and in bounds, testing at least 3 keys might be good?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +323,5 @@
> +    // gHistogramStringTable. They are stored in-order and consecutively,
> +    // from the offset key_index to (key_index + key_count).
> +    uint32_t string_offset = gHistogramKeyTable[this->key_index + i];
> +    const char* const str = &gHistogramStringTable[string_offset];
> +    if (key.EqualsASCII(str)) {

Optional: Skimming through dxr, it doesn't seem to be a problem to use `std::find_if()` here.

@@ +1654,5 @@
> +
> +  // Check if we're allowed to record in the provided key, for this histogram.
> +  if (!gHistograms[id].allows_key(NS_ConvertUTF16toUTF8(key))) {
> +    LogToBrowserConsole(nsIScriptError::errorFlag,
> +                        NS_LITERAL_STRING("Key not allowed for this keyed histogram"));

Let's print the histogram name & key to make this an actionable error.
Ditto below.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +162,5 @@
> +              !expectedKeyData.isUndefined())
> +    << "Cannot find the expected key in the histogram data";
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, "not-allowed", &expectedKeyData) &&
> +              expectedKeyData.isUndefined())
> +    << "Unallowed keys must not be recorded in the histogram data";

We should also test for the sum being as expected in both?

This is also testing a separate code path that also ends up tracking into "telemetry.accumulate_unknown_histogram_keys".
Should we test this?

Comment 36

[Alessio Placitelli [:Dexter]]

Untaking due to workweek and other priorities.

Comment 37

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Only rebased, carrying over r+

Comment 38

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Comment 39

[Alessio Placitelli [:Dexter]]

Comment on attachment 8900250 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

This was just rebasing + addressing comment 35.

(In reply to Georg Fritzsche [:gfritzsche] from comment #35)

> This is also testing a separate code path that also ends up tracking into
> "telemetry.accumulate_unknown_histogram_keys".
> Should we test this?

Yes, you're right. However, this is a bit more involving to do in gtest now: we need to expose/share the c++ scalar testing routines from TestScalar.cpp and use them here. How do you feel about me filing a new bug to add test coverage for that + move the testing functions in a new file/module?

Comment 40

[Georg Fritzsche [:gfritzsche]]

Sounds good.
Maybe lets use that chance to review our general testing approach and if we can make it less painful for us.

Comment 41

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af718604fae15e8a8d6908307eefac36756fc7d0

Comment 42

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1659e3dceab6d0d1847efee66161363d34ba17

Comment 43

[Alessio Placitelli [:Dexter]]

Attachment: Part 1 - Support 'keys' property for Histograms.json

Fixed linting.

Comment 44

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Support key whitelists in keyed histograms

Fixed build+linting.

Comment 45

[Alessio Placitelli [:Dexter]]

Comment on attachment 8900333 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

data-r?francois - Francois, this patch adds a new feature to the histogram data collection system, it lets telemetry users define a set of allowed keys in a key histogram.

To measure how many times non-allowed keys are being used, I'm defining a new opt-out scalar (see Scalars.yaml).

Comment 46

[François Marier [:francois]]

Comment on attachment 8900333 [details] [diff] [review]
Part 2 - Support key whitelists in keyed histograms

Review of attachment 8900333 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like Category 1 data.

datareview+

Comment 47

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/481f9c999735955352525516e2b4c21a33198636
Bug 1343855 - Part 1 - Support 'keys' property for Histograms.json. r=dexter

https://hg.mozilla.org/integration/mozilla-inbound/rev/17b77db08079a4783a6b74a2a9b92c3b4e3b5672
Bug 1343855 - Part 2 - Add support for key whitelists in keyed histograms. r=gfritzsche, data-review=francois

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/481f9c999735
https://hg.mozilla.org/mozilla-central/rev/17b77db08079