Implement keyed scalar measurements in Telemetry

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

(Blocks 2 bugs)

Trunk
mozilla52
Points:
3
Dependency tree / graph

Firefox Tracking Flags

(firefox49 wontfix, firefox50 wontfix, firefox51 fixed, firefox52 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(4 attachments, 12 obsolete attachments)

3.40 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
4.87 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
1.78 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
71.62 KB, patch
Dexter
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1295932
(Reporter)

Updated

3 years ago
Priority: P3 → P2
(Assignee)

Comment 1

3 years ago
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?
Flags: needinfo?(gfritzsche)
(Assignee)

Updated

3 years ago
Points: --- → 3
(Reporter)

Comment 2

3 years ago
(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.
Flags: needinfo?(gfritzsche)
(Assignee)

Updated

3 years ago
Priority: P2 → P1
(Assignee)

Updated

3 years ago
Assignee: nobody → alessio.placitelli
(Assignee)

Comment 3

3 years ago
This patch only takes care of updating the python scripts. The core changes and the docs will come as two additional different patches.
Attachment #8788432 - Flags: review?(gfritzsche)
(Reporter)

Comment 4

3 years ago
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."
Attachment #8788432 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 5

3 years ago
Attachment #8788432 - Attachment is obsolete: true
Attachment #8788815 - Flags: review+
(Assignee)

Comment 6

3 years ago
Georg, let me know if I should break down this patch a bit more.
Attachment #8788863 - Flags: review?(gfritzsche)
(Assignee)

Comment 7

3 years ago
Posted patch part 3 - Update the docs (obsolete) — Splinter Review
Attachment #8788864 - Flags: review?(gfritzsche)
(Reporter)

Comment 8

3 years ago
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.
Attachment #8788863 - Flags: review?(gfritzsche)
(Reporter)

Comment 9

3 years ago
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?
Attachment #8788864 - Flags: review?(gfritzsche) → feedback+
(Assignee)

Comment 10

3 years ago
(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.
Status: NEW → ASSIGNED
(Assignee)

Comment 11

3 years ago
Posted patch part 3 - Update the docs (obsolete) — Splinter Review
Attachment #8788864 - Attachment is obsolete: true
Attachment #8789342 - Flags: review?(gfritzsche)
(Assignee)

Comment 13

3 years ago
Attachment #8788863 - Attachment is obsolete: true
(Assignee)

Comment 14

3 years ago
(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?
(Assignee)

Updated

3 years ago
Attachment #8789344 - Flags: review?(gfritzsche)
(Reporter)

Comment 15

3 years ago
(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.
(Reporter)

Comment 16

3 years ago
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.
Attachment #8789342 - Flags: review?(gfritzsche) → review+
(Reporter)

Comment 17

3 years ago
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.
Attachment #8789344 - Flags: review?(gfritzsche)
(Reporter)

Updated

3 years ago
Attachment #8789343 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 18

3 years ago
Attachment #8789344 - Attachment is obsolete: true
(Assignee)

Comment 19

3 years ago
Attachment #8789342 - Attachment is obsolete: true
Attachment #8791120 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8791122 - Flags: review?(gfritzsche)
(Assignee)

Updated

3 years ago
Attachment #8791119 - Flags: review?(gfritzsche)
(Assignee)

Updated

3 years ago
Attachment #8791122 - Attachment is obsolete: true
Attachment #8791122 - Flags: review?(gfritzsche)
(Assignee)

Comment 22

3 years ago
As per IRC, I ended up folding the 5th patch into the 2nd. Sorry for the trouble.
Attachment #8791119 - Attachment is obsolete: true
Attachment #8791119 - Flags: review?(gfritzsche)
Attachment #8791129 - Flags: review?(gfritzsche)
(Assignee)

Comment 23

3 years ago
(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.
(Reporter)

Comment 24

3 years ago
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.");

<=
Attachment #8791129 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 25

3 years ago
Attachment #8791129 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Attachment #8791936 - Attachment is obsolete: true
(Assignee)

Comment 27

3 years ago
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.
Attachment #8791938 - Flags: review?(nfroyd)
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. :(
Attachment #8791938 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 29

3 years ago
Attachment #8791938 - Attachment is obsolete: true
Attachment #8792467 - Flags: review+
(Assignee)

Comment 30

3 years ago
(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.
(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?
(Assignee)

Comment 33

3 years ago
(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.
Flags: needinfo?(nfroyd)
(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.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 35

3 years ago
Attachment #8792467 - Attachment is obsolete: true
Attachment #8793295 - Flags: review+
(Assignee)

Comment 36

3 years ago
(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).
Is this a good candidate for uplift to aurora?
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 41

3 years ago
(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?
Flags: needinfo?(alessio.placitelli) → needinfo?(lhenry)
(Assignee)

Updated

3 years ago
Depends on: 1305654
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!
(Assignee)

Comment 43

3 years ago
(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 ;)
Given that the dev team prefers to let this ride the trains, it's a wontfix for 50 and 51.
(Assignee)

Comment 45

3 years ago
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
Attachment #8788815 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8793295 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8791121 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

3 years ago
Attachment #8791120 - Flags: approval-mozilla-aurora?
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.
Attachment #8788815 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8791120 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8791121 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8793295 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

a year ago
Depends on: 1458463
You need to log in before you can comment on or make changes to this bug.