1277806 - Implement keyed scalar measurements in Telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

After we got the initial scalar support in that is needed to support the engagement measurements, we should add support for "keyed scalars" to e.g. support measurements like search counts.

Comment 1

[Alessio Placitelli [:Dexter]]

Georg, what do you think of the following API?

- JS -

Services.telemetry.keyedScalarAdd(aName, aKey, aValue);
Services.telemetry.keyedScalarSet(aName, aKey, aValue);
Services.telemetry.keyedScalarSetMaximum(aName, aKey, aValue);

- C++ -

void KeyedScalarAdd(mozilla::Telemetry::ScalarID aId, const nsCString& aKey, uint32_t aValue);
void KeyedScalarSet(mozilla::Telemetry::ScalarID aId, const nsCString& aKey, uint32_t aValue);
void KeyedScalarSet(mozilla::Telemetry::ScalarID aId, const nsCString& aKey, bool aValue);
void KeyedScalarSetMaximum(mozilla::Telemetry::ScalarID aId, const nsCString& aKey, uint32_t aValue);

Do we have an use case for keyed string scalars? Does it make sense to allow them?

Comment 2

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #1)
> void KeyedScalarAdd(mozilla::Telemetry::ScalarID aId, const nsCString& aKey,
> uint32_t aValue);
...

I think these can just be overloads, `ScalarAdd(id, key, value)` etc.

> Do we have an use case for keyed string scalars? Does it make sense to allow
> them?

I can't think of a need for it now. Maybe we should just not do them until someone brings up a use case.

Comment 3

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Update the python scripts

This patch only takes care of updating the python scripts. The core changes and the docs will come as two additional different patches.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8788432 [details] [diff] [review]
part 1 - Update the python scripts

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

::: toolkit/components/telemetry/parse_scalars.py
@@ +194,5 @@
>          return self._definition['kind']
>  
>      @property
> +    def keyed(self):
> +        """Get the scalar keyed property"""

That doesn't seem to explain the property.
Maybe: "Boolean indicating whether this is a keyed scalar."

Comment 5

[Alessio Placitelli [:Dexter]]

Attachment: part 1 - Update the python scripts

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Georg, let me know if I should break down this patch a bit more.

Comment 7

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the docs

Comment 8

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8788863 [details] [diff] [review]
part 2 - Implement the Keyed Scalars core

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

::: toolkit/components/telemetry/Telemetry.h
@@ +363,5 @@
>   *
>   * @param aId The scalar enum id.
> + * @param aValue The boolean value to set the scalar to.
> + */
> +void ScalarSet(mozilla::Telemetry::ScalarID aId, bool aValue);

Why is this change in this patch?
Should this be keyed?

@@ +399,5 @@
> + * @param aId The scalar enum id.
> + * @param aKey The scalar key.
> + * @param aValue The numeric, unsigned value to set the scalar to.
> + */
> +void ScalarSet(mozilla::Telemetry::ScalarID aId, const nsAString& aKey, uint32_t aValue);

Are you intentionally only adding `uint32_t` support for keyed histograms?
What about `bool`?

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +997,5 @@
> +
> +    return ret;
> +  },
> +
> +  getKeyedScalars: function (subsession, clearSubsession) {

The above function already handles snapshotting keyed scalars, what is this one for?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +443,5 @@
> +   * @param aName The scalar name.
> +   * @param aKey The key name.
> +   * @param aValue The value to set the scalar to. If the type of aValue doesn't match the
> +   *        type of the scalar, the function will fail. For scalar string types, the this
> +   *        is truncated to 50 characters.

Did we decide to also support keyed string scalars?
I thought we'd only do numbers and bools right now.

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8788864 [details] [diff] [review]
part 3 - Update the docs

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

::: toolkit/components/telemetry/docs/collection/scalars.rst
@@ +24,5 @@
>    Services.telemetry.scalarSetMaximum(aName, aValue);
>  
> +  Services.telemetry.keyedScalarAdd(aName, aKey, aValue);
> +  Services.telemetry.keyedScalarSet(aName, aKey, aValue);
> +  Services.telemetry.keyedScalarSetMaximum(aName, aKey, aValue);

I think this file is missing short code examples for plain & keyed, JS & C++ usage.
Lets take this to a follow-up bug.

@@ +34,5 @@
>  C++ API
>  -------
>  Probes in native code can use the more convenient helper functions declared in `Telemetry.h <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.h>`_:
>  
>  .. code-block:: cpp

Side note: I wonder if we could include documentation groups from doc comments in the future.

@@ +114,5 @@
>  to set a longer string will result in an error and no string being set.
>  
> +Keyed Scalars
> +-------------
> +Keyed scalars are collections of one of the available scalar types, indexed by a string key. This is for example useful when you want to break down certain counts by a name, like how often searches happen with which search engine.

We should make it clear that this is only useful if the set of keys are not known beforehand.
If the keys are from a known set of strings, other options are preferred if suitable, like categorical histograms or splitting measurements up into separate scalars.

Maybe we should add categorical scalars, e.g. by extending keyed scalars to specify a known set of keys for these scenarios.
Can you file a follow-up on that and involve me and Roberto there?

Comment 10

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #9)
> Comment on attachment 8788864 [details] [diff] [review]
> part 3 - Update the docs
> 
> Review of attachment 8788864 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/docs/collection/scalars.rst
> @@ +24,5 @@
> >    Services.telemetry.scalarSetMaximum(aName, aValue);
> >  
> > +  Services.telemetry.keyedScalarAdd(aName, aKey, aValue);
> > +  Services.telemetry.keyedScalarSet(aName, aKey, aValue);
> > +  Services.telemetry.keyedScalarSetMaximum(aName, aKey, aValue);
> 
> I think this file is missing short code examples for plain & keyed, JS & C++
> usage.
> Lets take this to a follow-up bug.

Filed bug 1301364.
 
> @@ +114,5 @@
> >  to set a longer string will result in an error and no string being set.
> >  
> > +Keyed Scalars
> > +-------------
> > +Keyed scalars are collections of one of the available scalar types, indexed by a string key. This is for example useful when you want to break down certain counts by a name, like how often searches happen with which search engine.
> 
> We should make it clear that this is only useful if the set of keys are not
> known beforehand.
> If the keys are from a known set of strings, other options are preferred if
> suitable, like categorical histograms or splitting measurements up into
> separate scalars.
> 
> Maybe we should add categorical scalars, e.g. by extending keyed scalars to
> specify a known set of keys for these scenarios.
> Can you file a follow-up on that and involve me and Roberto there?

Filed bug 1301365.

Comment 11

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the docs

Comment 12

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Bonus: add the missing C++ APi for setting boolean scalars

Comment 13

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Comment on attachment 8788863 [details] [diff] [review]
> part 2 - Implement the Keyed Scalars core
> 
> Review of attachment 8788863 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Telemetry.h
> @@ +363,5 @@
> >   *
> >   * @param aId The scalar enum id.
> > + * @param aValue The boolean value to set the scalar to.
> > + */
> > +void ScalarSet(mozilla::Telemetry::ScalarID aId, bool aValue);
> 
> Why is this change in this patch?
> Should this be keyed?

I noticed this was missing for plain scalars: I moved the chunk to a separate patch.

> @@ +399,5 @@
> > + * @param aId The scalar enum id.
> > + * @param aKey The scalar key.
> > + * @param aValue The numeric, unsigned value to set the scalar to.
> > + */
> > +void ScalarSet(mozilla::Telemetry::ScalarID aId, const nsAString& aKey, uint32_t aValue);
> 
> Are you intentionally only adding `uint32_t` support for keyed histograms?
> What about `bool`?

Forgot to add it :) Added it now

> ::: toolkit/components/telemetry/TelemetrySession.jsm
> @@ +997,5 @@
> > +
> > +    return ret;
> > +  },
> > +
> > +  getKeyedScalars: function (subsession, clearSubsession) {
> 
> The above function already handles snapshotting keyed scalars, what is this
> one for?

My bad, that's a bogus function from an older version of the patch.

> ::: toolkit/components/telemetry/nsITelemetry.idl
> @@ +443,5 @@
> > +   * @param aName The scalar name.
> > +   * @param aKey The key name.
> > +   * @param aValue The value to set the scalar to. If the type of aValue doesn't match the
> > +   *        type of the scalar, the function will fail. For scalar string types, the this
> > +   *        is truncated to 50 characters.
> 
> Did we decide to also support keyed string scalars?
> I thought we'd only do numbers and bools right now.

Whoops, changed the doc string. This currently allows using keyed scalar strings from the JS interface (but it lacks the implementation in the C++ API). Should we manually disallow string scalars in the JS code paths?

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Alessio Placitelli [:Dexter] from comment #14)
...
> > > +   *        type of the scalar, the function will fail. For scalar string types, the this
> > > +   *        is truncated to 50 characters.
> > 
> > Did we decide to also support keyed string scalars?
> > I thought we'd only do numbers and bools right now.
> 
> Whoops, changed the doc string. This currently allows using keyed scalar
> strings from the JS interface (but it lacks the implementation in the C++
> API). Should we manually disallow string scalars in the JS code paths?

Yes, as we do with other unsupported operations.

Comment 16

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8789342 [details] [diff] [review]
part 3 - Update the docs

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

This is missing documentation of the restrictions on the key length, allowed characters & key counts.
r=me with that added.

::: toolkit/components/telemetry/docs/collection/scalars.rst
@@ +30,3 @@
>  These functions can throw if, for example, an operation is performed on a scalar type that doesn't support it
>  (e.g. calling scalarSetMaximum on a scalar of the string kind). Please look at the code documentation for
>  additional informations.

While we're here:
* link to code documentation
* s/informations/information/

@@ +105,5 @@
>  ---------------
>  
>  - ``cpp_guard``: A string that gets inserted as an ``#ifdef`` directive around the automatically generated C++ declaration. This is typically used for platform-specific scalars, e.g. ``ANDROID``.
>  - ``release_channel_collection``: This can be either ``opt-in`` (default) or ``opt-out``. With the former the scalar is submitted by default on pre-release channels; on the release channel only if the user opted into additional data collection. With the latter the scalar is submitted by default on release and pre-release channels, unless the user opted out.
> +- ``keyed``: A boolean that determines whether this is a keyed scalar. It defaults to False.

Nit: ``False``

@@ +114,5 @@
>  to set a longer string will result in an error and no string being set.
>  
> +Keyed Scalars
> +-------------
> +Keyed scalars are collections of one of the available scalar types, indexed by a string key. This is for example useful when you want to break down certain counts by a name, like how often searches happen with which search engine. Keyed scalars should only be used if the set of keys are not known beforehand.

Nit: Move "Keyed scalars should ..." to a separate paragraph with the following sentence.

Comment 17

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8789344 [details] [diff] [review]
part 2 - Implement the Keyed Scalars core

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

Further review comments below, but i'm also missing some sanity limits for the keys here:
(1) We should restrict the key length to a sane maximum here.
Per bug 1275035, comment 2, 70 sounds reasonable.
This should probably be:
* MOZ_ASSERT() + NS_WARN_IF() for the C++ path
* console error for the JS path
(2) Also, we should restrict the number of keys. Printing errors and dropping any new keys should be sufficient in that case.
... let's do that in another patch here.

::: toolkit/components/telemetry/Telemetry.cpp
@@ +3051,5 @@
> + *
> + * @param aId The scalar enum id.
> + * @param aKey The scalar key.
> + * @param aValue The unsigned value to add to the scalar.
> + */

Side-note: Do we need to repeated the docs from the headers here?

::: toolkit/components/telemetry/Telemetry.h
@@ +380,5 @@
> + * Adds the value to the given scalar.
> + *
> + * @param aId The scalar enum id.
> + * @param aKey The scalar key.
> + * @param aValue The unsigned value to add to the scalar.

"unsigned" should be clear from the type already.
Ditto for the other comments below.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +529,5 @@
> +  size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);
> +
> +private:
> +  typedef nsBaseHashtableET<nsCStringHashKey, ScalarBase*> KeyedScalarEntry;
> +  typedef AutoHashtable<KeyedScalarEntry> ScalarKeysMapType;

This will leak the `ScalarBase` instances. This should probably be a `nsClassHashtable`.

@@ +543,5 @@
> +{
> +  ScalarBase* scalar = nullptr;
> +  nsresult rv = GetScalarForKey(aKey, &scalar);
> +  if (NS_FAILED(rv)) {
> +    return ScalarResult::InvalidType;

Is that the only possible failure reason?
Ditto the functions below.

@@ +665,5 @@
> +    *aRet = entry->mData;
> +    return NS_OK;
> +  }
> +
> +  ScalarBase* scalar = internal_ScalarAllocate(mScalarInfo);

This is the only use for `mScalarInfo`. So, the only information we need to store & pass is the scalars type.

@@ -598,5 @@
>  //
>  // PRIVATE: thread-unsafe helpers for the external interface
>  
>  namespace {
> -

Unneeded change?

@@ +852,5 @@
> +bool
> +internal_IsKeyedScalar(mozilla::Telemetry::ScalarID aId)
> +{
> +  const ScalarInfo &info = internal_InfoForScalarID(aId);
> +  return info.keyed;

Nit: Just `return internal_...().keyed`?

@@ +1001,5 @@
> + */
> +nsresult
> +internal_GetKeyedScalarByEnum(mozilla::Telemetry::ScalarID aId, KeyedScalar** aRet)
> +{
> +  if (!IsValidEnumId(aId)) {

Let's `MOZ_ASSERT()` this too. Would be great to also add it for the non-keyed path.

@@ +1196,5 @@
> + * @param aName The scalar name.
> + * @param aKey The key name.
> + * @param aVal The numeric value to add to the scalar.
> + * @param aCx The JS context.
> + * @return NS_OK if the value was added or if we're not allow to record to this

Nit: Allowed.

@@ +1761,5 @@
>  }
>  
>  /**
> + * Serializes the scalars from the given dataset to a json-style object and resets them.
> + * The returned structure looks like {"group1.probe":1,"group1.other_probe":false,...}.

The structure is different for keyed scalars.

@@ +1786,5 @@
> +  typedef mozilla::Pair<const char*, nsTArray<KeyedScalar::KeyValuePair>> DataPair;
> +  nsTArray<DataPair> scalarsToReflect;
> +  {
> +    StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> +    // Iterate the scalars in gScalarStorageMap. The storage may contain empty or yet to be

gKeyedScalarStorageMap

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +965,5 @@
>    },
>  
> +  /**
> +   * Get a snapshot of the scalars and clear them.
> +   * @param {subsession} If true, we are in a subsession.

Nit: "If true, then we collect the data for a subsession."

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +332,5 @@
>    checkValue(OPTOUT_SCALAR, 5);
>    checkValue(OPTIN_SCALAR, 6);
>  });
>  
> +add_task(function* test_keyedScalarRecording() {

I'm missing a test for enforcing a length limit on the key strings.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +251,5 @@
> +
> +  let checkScalar = function(scalar) {
> +    // Check if the value is of a supported type.
> +    const valueType = typeof(scalar);
> +    if (valueType === "string") {

This calls for `switch (valueType) ...`.

@@ +278,5 @@
> +  const keyedScalars = parentProcess.keyedScalars;
> +  for (let name in keyedScalars) {
> +    Assert.equal(typeof name, "string", "Scalar names must be strings.");
> +    for (let key in keyedScalars[name]) {
> +      Assert.equal(typeof key, "string", "Keyed scalar keys must be strings.");

Also check the key length limit.

Comment 18

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 19

[Alessio Placitelli [:Dexter]]

Attachment: part 3 - Update the docs

Comment 20

[Alessio Placitelli [:Dexter]]

Attachment: part 4 - Bonus: add the missing C++ APi for setting boolean scalars

Rebased.

Comment 21

[Alessio Placitelli [:Dexter]]

Attachment: part 5 - Limit the number of keys in the keyed scalars.

Comment 22

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

As per IRC, I ended up folding the 5th patch into the 2nd. Sorry for the trouble.

Comment 23

[Alessio Placitelli [:Dexter]]

(In reply to Georg Fritzsche [:gfritzsche] [away Sep 10 - 12] from comment #17)
> @@ +543,5 @@
> > +{
> > +  ScalarBase* scalar = nullptr;
> > +  nsresult rv = GetScalarForKey(aKey, &scalar);
> > +  if (NS_FAILED(rv)) {
> > +    return ScalarResult::InvalidType;
> 
> Is that the only possible failure reason?
> Ditto the functions below.

Yes, it was. I've changed the code a bit to use ScalarResult though, as new failure reasons were added to support the max key length and number cases.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8791129 [details] [diff] [review]
part 2 - Implement the Keyed Scalars core

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

Higher-level, this looks fine to me now.
r=me overall with the below fixed, but after updating the patch i'd want froydnj to have a look (f? or r?) over the C++ implementation details in TelemetryScalar.cpp specifically.
Please explicitly call out that this doesn't need review outside of TelemetryScalar.cpp and is fine from the Telemetry functionality perspective.

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +530,5 @@
> +  void SetMaximum(const nsAString& aKey, uint32_t aValue);
> +
> +  // GetValue is used to get the key-value pairs stored in the keyed scalar
> +  // when persisting it to JS.
> +  nsresult GetValue(nsTArray<KeyValuePair>& aValues);

const?

@@ +539,5 @@
> +private:
> +  typedef nsClassHashtable<nsCStringHashKey, ScalarBase> ScalarKeysMapType;
> +
> +  ScalarKeysMapType mScalarKeys;
> +  uint32_t mScalarKind;

const?

@@ +587,5 @@
> +  ScalarBase* scalar = nullptr;
> +  ScalarResult sr = GetScalarForKey(aKey, &scalar);
> +  if (sr != ScalarResult::Ok) {
> +    if (NS_WARN_IF(sr == ScalarResult::KeyTooLong)) {
> +      MOZ_ASSERT(false, "The key length must be limited to 70 characters.");

These checks are repeated multiple times.
Please do them once in `GetScalarForKey()`?

@@ +807,5 @@
>  internal_ShouldLogError(ScalarResult aSr)
>  {
>    if (aSr == ScalarResult::StringTooLong ||
> +      aSr == ScalarResult::KeyTooLong ||
> +      aSr == ScalarResult::TooManyKeys ||

`switch (aSr) ...`?

@@ +1804,5 @@
> + * The returned structure looks like:
> + *   { "group1.probe": { "key_1": 2, "key_2": 1, ... }, ... }
> + *
> + * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN.
> + * @param aClear Whether to clear out the scalars after snapshotting.

... the keyed scalars after ...

::: toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ +63,5 @@
> +            KEYED_UINT_SCALAR + " must be serialized with the keyed scalars.");
> +  Assert.ok(expectedKey in keyedScalars[KEYED_UINT_SCALAR],
> +            KEYED_UINT_SCALAR + " must contain the expected keys.");
> +  Assert.ok(expectedOtherKey in keyedScalars[KEYED_UINT_SCALAR],
> +            KEYED_UINT_SCALAR + " must contain the expected keys.");

This asserts that the scalar has at least these two keys.
We should assert that it has exactly these two keys.

@@ +516,5 @@
> +    Telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN);
> +  Assert.equal(keyedScalars[KEYED_UINT_SCALAR][NORMAL_KEY], 1,
> +               "The key must contain the expected value.");
> +  Assert.ok(!(LONG_KEY_STRING in keyedScalars[KEYED_UINT_SCALAR]),
> +            "The data for the long key should not have been recorded.");

Assert that KEYED_UINT_SCALAR has exactly one key, which should be NORMAL_KEY.

@@ +524,5 @@
> +  Telemetry.clearScalars();
> +
> +  // Add 100 keys to an histogram and set their initial value.
> +  for (let k = 0; k < 100; k++) {
> +    const KEY_NAME = "key_" + k;

Not a required change:
If you generate a Set() of keys here...

@@ +525,5 @@
> +
> +  // Add 100 keys to an histogram and set their initial value.
> +  for (let k = 0; k < 100; k++) {
> +    const KEY_NAME = "key_" + k;
> +    Telemetry.keyedScalarSet(KEYED_UINT_SCALAR, KEY_NAME, k);

... populate the scalar with the keys from the set here...

@@ +549,5 @@
> +
> +  for (let k = 0; k < 100; k++) {
> +    const KEY_NAME = "key_" + k;
> +    Assert.equal(keyedScalars[KEYED_UINT_SCALAR][KEY_NAME], k,
> +                 "The key must contain the expected value.");

... then you can do `.equal(Set(Object.keys(...)), keySet, ...)` instead of regenerating the names.

@@ +553,5 @@
> +                 "The key must contain the expected value.");
> +  }
> +
> +  Assert.ok(!(LAST_KEY_NAME in keyedScalars[KEYED_UINT_SCALAR]),
> +            "The keys over 100 must not have been recorded.");

... and this check becomes redundant.

::: toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ +280,5 @@
> +
> +  // Check that we have valid keyed scalar entries.
> +  const keyedScalars = parentProcess.keyedScalars;
> +  for (let name in keyedScalars) {
> +    Assert.equal(typeof name, "string", "Scalar names must be strings.");

Also assert on `Object.keys(keyedScalars[name]).length`.

@@ +283,5 @@
> +  for (let name in keyedScalars) {
> +    Assert.equal(typeof name, "string", "Scalar names must be strings.");
> +    for (let key in keyedScalars[name]) {
> +      Assert.equal(typeof key, "string", "Keyed scalar keys must be strings.");
> +      Assert.ok(key.length < 70, "Keyed scalar keys can't have more than 70 characters.");

<=

Comment 25

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 26

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 27

[Alessio Placitelli [:Dexter]]

Comment on attachment 8791938 [details] [diff] [review]
part 2 - Implement the Keyed Scalars core

Nathan, could you take a look at the C++ changes details in TelemetryScalars.cpp?

As called out in comment 24, this doesn't need a review outside of that file and is fine from the Telemetry functionality perspective.

Comment 28

[Nathan Froyd [:froydnj]]

Comment on attachment 8791938 [details] [diff] [review]
part 2 - Implement the Keyed Scalars core

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

r=me, I guess. Do you want the MOZ_ASSERT(false, ...) scattered throughout to be MOZ_CRASH(...)?

::: toolkit/components/telemetry/TelemetryScalar.cpp
@@ +675,5 @@
> +  if (mScalarKeys.Count() >= kMaximumNumberOfKeys) {
> +    return ScalarResult::TooManyKeys;
> +  }
> +
> +  nsCString utf8Key = NS_ConvertUTF16toUTF8(aKey);

Please just:

NS_ConvertUTF16toUTF8 utf8Key(aKey);

@@ +795,5 @@
> +    case ScalarResult::KeyTooLong:
> +    case ScalarResult::TooManyKeys:
> +    case ScalarResult::UnsignedNegativeValue:
> +    case ScalarResult::UnsignedTruncatedValue:
> +      // Intentional fall-through.

You may have to annotate the fallthrough cases with MOZ_FALLTHROUGH.  Perhaps that only applies if there's actual code involved in the case statement, though.

@@ +1712,5 @@
> +                            uint32_t aValue)
> +{
> +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> +
> +  KeyedScalar* scalar = internal_GetRecordableKeyedScalar(aId);

Why are we not checking whether we're allowed to record this scalar in this codepath?

::: toolkit/components/telemetry/nsITelemetry.idl
@@ +431,5 @@
> +   * Adds the value to the given keyed scalar.
> +   *
> +   * @param aName The scalar name.
> +   * @param aKey The key name.
> +   * @param aValue The numeric value to add to the scalar. Only unsigned integers supported.

If we're going to specify that, can we just have unsigned int in the IDL and not have to worry about converting the value ourselves?  Likewise for all of these, and then we can ditch the implicit_jscontext, too.

Oh.  I guess the scalar stuff doesn't conveniently support that.  So sad. :(

Comment 29

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 30

[Alessio Placitelli [:Dexter]]

(In reply to Nathan Froyd [:froydnj] from comment #28)
> Comment on attachment 8791938 [details] [diff] [review]
> part 2 - Implement the Keyed Scalars core
> 
> Review of attachment 8791938 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, I guess. Do you want the MOZ_ASSERT(false, ...) scattered throughout
> to be MOZ_CRASH(...)?

Thanks for the review, Nathan.
I think we'd want MOZ_ASSERT to stay, as that would be consistent with the rest of the Telemetry modules (Telemetry.cpp/TelemetrHistograms.cpp) which only use MOZ_CRASH occasionally, as it would be crashing also in Release.

> @@ +1712,5 @@
> > +                            uint32_t aValue)
> > +{
> > +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> > +
> > +  KeyedScalar* scalar = internal_GetRecordableKeyedScalar(aId);
> 
> Why are we not checking whether we're allowed to record this scalar in this
> codepath?

That's done inside |internal_GetRecordableKeyedScalar|, by |internal_CanRecordForScalarID|.

> ::: toolkit/components/telemetry/nsITelemetry.idl
> @@ +431,5 @@
> > +   * Adds the value to the given keyed scalar.
> > +   *
> > +   * @param aName The scalar name.
> > +   * @param aKey The key name.
> > +   * @param aValue The numeric value to add to the scalar. Only unsigned integers supported.
> 
> If we're going to specify that, can we just have unsigned int in the IDL and
> not have to worry about converting the value ourselves?  Likewise for all of
> these, and then we can ditch the implicit_jscontext, too.
> 
> Oh.  I guess the scalar stuff doesn't conveniently support that.  So sad. :(

Yeah :-\ I'm not sure it's convenient for us to do that now, unless there's a very good reason to change that.

Comment 31

[Alessio Placitelli [:Dexter]]

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

Comment 32

[Nathan Froyd [:froydnj]]

(In reply to Alessio Placitelli [:Dexter] from comment #30)
> > @@ +1712,5 @@
> > > +                            uint32_t aValue)
> > > +{
> > > +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> > > +
> > > +  KeyedScalar* scalar = internal_GetRecordableKeyedScalar(aId);
> > 
> > Why are we not checking whether we're allowed to record this scalar in this
> > codepath?
> 
> That's done inside |internal_GetRecordableKeyedScalar|, by
> |internal_CanRecordForScalarID|.

Oh.  Why aren't the JS paths using this same function, then?

Comment 33

[Alessio Placitelli [:Dexter]]

(In reply to Nathan Froyd [:froydnj] from comment #32)
> (In reply to Alessio Placitelli [:Dexter] from comment #30)
> > > @@ +1712,5 @@
> > > > +                            uint32_t aValue)
> > > > +{
> > > > +  StaticMutexAutoLock locker(gTelemetryScalarsMutex);
> > > > +
> > > > +  KeyedScalar* scalar = internal_GetRecordableKeyedScalar(aId);
> > > 
> > > Why are we not checking whether we're allowed to record this scalar in this
> > > codepath?
> > 
> > That's done inside |internal_GetRecordableKeyedScalar|, by
> > |internal_CanRecordForScalarID|.
> 
> Oh.  Why aren't the JS paths using this same function, then?

Because we want a much more granular control on the return values of each of the internal_* functions for the JS parts.

If you have a strong opinion on that, I can hold back this patch from landing and address your concerns.

Comment 34

[Nathan Froyd [:froydnj]]

(In reply to Alessio Placitelli [:Dexter] from comment #33)
> If you have a strong opinion on that, I can hold back this patch from
> landing and address your concerns.

I do not have a strong opinion on that.  Go ahead and land this.

Comment 35

[Alessio Placitelli [:Dexter]]

Attachment: part 2 - Implement the Keyed Scalars core

Comment 36

[Alessio Placitelli [:Dexter]]

(In reply to Nathan Froyd [:froydnj] from comment #34)
> (In reply to Alessio Placitelli [:Dexter] from comment #33)
> > If you have a strong opinion on that, I can hold back this patch from
> > landing and address your concerns.
> 
> I do not have a strong opinion on that.  Go ahead and land this.

Thanks.

Note: I've updated the patch to fix the broken builds on try (made the costructor for KeyedScalars explicit).

Comment 37

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f30957884c9&selectedJob=27798833

Comment 38

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/fx-team/rev/66a2fa62d7601aaae4069bc9a2c4ec13854f65e7
Bug 1277806 - Add support for the keyed scalars to the scalar registry. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/d99209b752fbc75b35e437a476c14a93f54b0774
Bug 1277806 - Implement keyed scalar measurements in Telemetry. r=gfritzsche,nfroyd

https://hg.mozilla.org/integration/fx-team/rev/d0ad8d0da0ef0c0a7fdfeacdf3f8622531baed45
Bug 1277806 - Update the docs. r=gfritzsche

https://hg.mozilla.org/integration/fx-team/rev/73c0774056bb42c4c5a27f65e01a24613ab7dd7c
Bug 1277806 - Add the missing C++ API for boolean scalars. r=gfritzsche

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66a2fa62d760
https://hg.mozilla.org/mozilla-central/rev/d99209b752fb
https://hg.mozilla.org/mozilla-central/rev/d0ad8d0da0ef
https://hg.mozilla.org/mozilla-central/rev/73c0774056bb

Comment 40

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this a good candidate for uplift to aurora?

Comment 41

[Alessio Placitelli [:Dexter]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #40)
> Is this a good candidate for uplift to aurora?

I'd love this to raid the trains normally, so I'm inclined to say "no".
Unless there's a reason to move this forward fast :-D Is there a reason to do so?

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Nope, just checking, since it shows up on our list of bugs fixed in the current nightly, marked as affected in an earlier version. We try to catch  missed uplift opportunities this way. Thanks!

Comment 43

[Alessio Placitelli [:Dexter]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #42)
> Nope, just checking, since it shows up on our list of bugs fixed in the
> current nightly, marked as affected in an earlier version. We try to catch 
> missed uplift opportunities this way. Thanks!

Nice, I didn't know that system was in place, thanks for asking ;)

Comment 44

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that the dev team prefers to let this ride the trains, it's a wontfix for 50 and 51.

Comment 45

[Alessio Placitelli [:Dexter]]

Comment on attachment 8788815 [details] [diff] [review]
part 1 - Update the python scripts

Approval Request Comment
[Feature/regressing bug #]: Bug 1303333 (search scalar measurement)
[User impact if declined]: No user impact, but we won't be able to gather new search data that will be uplifted with bug 1303333.
[Describe test coverage new/current, TreeHerder]: This series of patches have lived for quite a while on m-c with no problems, plus they add new test coverage.
[Risks and why]: Low risk, there were no failures or intermittents associated to this on m-c. There is some risk involved in changing the Telemetry c++ core, but we've been validating the data coming in since this change landed and there was no problem.
[String/UUID change made/needed]: None

Comment 46

[Gerry Chang [:gchang]]

Comment on attachment 8788815 [details] [diff] [review]
part 1 - Update the python scripts

The patch helps gather data for search and has been in m-c for a while. We can take it in 51 aurora.

Comment 47

[Alessio Placitelli [:Dexter]]

Aurora changesets:

https://hg.mozilla.org/releases/mozilla-aurora/rev/58cf57f2c147a9a52c8075d5b6bf1a769616c69b
https://hg.mozilla.org/releases/mozilla-aurora/rev/5474dd7024a36e244a4e36ca5eb0b26afe1f8ffe
https://hg.mozilla.org/releases/mozilla-aurora/rev/a799212022f7f7458cb2f972d31a0b0e8fa31037
https://hg.mozilla.org/releases/mozilla-aurora/rev/7cfedc6abf05f75ece46143e9af6cad41cfcea0e