Bug 1365529 Allow restricting scalar keys to a known set in Scalars.yaml r=chutten

Differential Revision: https://phabricator.services.mozilla.com/D51629

--HG--
extra : moz-landing-system : lando

toolkit/components/telemetry/Scalars.yaml

@@ -2635,6 +2635,28 @@ telemetry:
     record_in_processes:
       - 'main'
 
+  keyed_scalars_unknown_keys:
+    bug_numbers:
+      - 1365529
+    description: >
+      The count of attempted accumulations to unknown scalar keys for
+      scalars that restrict the set of allowed keys ('keys' property).
+      The names of the offending scalars are used as keys in this probe.
+    expires: never
+    kind: uint
+    keyed: true
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+      - brizental@mozilla.com
+    release_channel_collection: opt-out
+    products:
+      - 'firefox'
+      - 'fennec'
+      - 'geckoview'
+      - 'thunderbird'
+    record_in_processes:
+      - all
+
   about_telemetry_pageload:
     bug_numbers:
       - 1512503
@@ -5179,6 +5201,28 @@ telemetry.test:
     record_in_processes:
       - 'main'
 
+  keyed_with_keys:
+    bug_numbers:
+      - 1277806
+    description: A testing keyed scalar with defined keys; not meant to be touched.
+    expires: never
+    kind: uint
+    keyed: true
+    keys:
+      - 'only'
+      - 'meant'
+      - 'for'
+      - 'testing'
+    notification_emails:
+      - telemetry-client-dev@mozilla.com
+    products:
+      - 'firefox'
+      - 'fennec'
+      - 'geckoview'
+      - 'thunderbird'
+    record_in_processes:
+      - 'main'
+
   child_keyed_unsigned_int:
     bug_numbers:
       - 1466490

toolkit/components/telemetry/build_scripts/gen_scalar_data.py

@@ -36,7 +36,8 @@ file_footer = """\
 """
 
 
-def write_scalar_info(scalar, output, name_index, expiration_index, store_index, store_count):
+def write_scalar_info(scalar, output, name_index, expiration_index, store_index, store_count,
+                      key_count, key_index):
     """Writes a scalar entry to the output file.
 
     :param scalar: a ScalarType instance describing the scalar.
@@ -45,13 +46,15 @@ def write_scalar_info(scalar, output, name_index, expiration_index, store_index,
     :param expiration_index: the index of the expiration version in the strings table.
     """
     if scalar.record_on_os(buildconfig.substs["OS_TARGET"]):
-        print("  {{ {}, {}, {}, {}, {}, {}, {}, {}, {} }},"
+        print("  {{ {}, {}, {}, {}, {}, {}, {}, {}, {}, {}, {} }},"
               .format(scalar.nsITelemetry_kind,
                       name_index,
                       expiration_index,
                       scalar.dataset,
                       " | ".join(scalar.record_in_processes_enum),
                       "true" if scalar.keyed else "false",
+                      key_count,
+                      key_index,
                       " | ".join(scalar.products_enum),
                       store_count,
                       store_index),
@@ -69,6 +72,9 @@ def write_scalar_tables(scalars, output):
     store_table = []
     total_store_count = 0
 
+    keys_table = []
+    total_key_count = 0
+
     print("const ScalarInfo gScalars[] = {", file=output)
     for s in scalars:
         # We add both the scalar label and the expiration string to the strings
@@ -86,8 +92,16 @@ def write_scalar_tables(scalars, output):
             store_table.append((s.label, string_table.stringIndexes(stores)))
             total_store_count += len(stores)
 
+        keys = s.keys
+        key_index = 0
+        if len(keys) > 0:
+            key_index = total_key_count
+            keys_table.append((s.label, string_table.stringIndexes(keys)))
+            total_key_count += len(keys)
+
         # Write the scalar info entry.
-        write_scalar_info(s, output, name_index, exp_index, store_index, len(stores))
+        write_scalar_info(s, output, name_index, exp_index, store_index, len(stores),
+                          len(keys), key_index)
     print("};", file=output)
 
     string_table_name = "gScalarsStringTable"
@@ -95,6 +109,11 @@ def write_scalar_tables(scalars, output):
     static_assert(output, "sizeof(%s) <= UINT32_MAX" % string_table_name,
                   "index overflow")
 
+    print("\nconstexpr uint32_t gScalarKeysTable[] = {", file=output)
+    for name, indexes in keys_table:
+        print("/* %s */ %s," % (name, ", ".join(map(str, indexes))), file=output)
+    print("};", file=output)
+
     store_table_name = "gScalarStoresTable"
     print("\n#if defined(_MSC_VER) && !defined(__clang__)", file=output)
     print("const uint32_t {}[] = {{".format(store_table_name), file=output)
@@ -139,6 +158,7 @@ def generate_JSON_definitions(output, *filenames):
         scalar_definitions[category][scalar.name] = OrderedDict({
             'kind': scalar.nsITelemetry_kind,
             'keyed': scalar.keyed,
+            'keys': scalar.keys,
             'record_on_release': True if scalar.dataset_short == 'opt-out' else False,
             # We don't expire dynamic-builtin scalars: they're only meant for
             # use in local developer builds anyway. They will expire when rebuilding.

toolkit/components/telemetry/build_scripts/mozparsers/parse_scalars.py

@@ -110,6 +110,7 @@ class ScalarType:
         OPTIONAL_FIELDS = {
             'release_channel_collection': basestring,
             'keyed': bool,
+            'keys': list,
             'operating_systems': list,
             'record_into_store': list,
         }
@@ -120,6 +121,7 @@ class ScalarType:
             'notification_emails': basestring,
             'record_in_processes': basestring,
             'products': basestring,
+            'keys': basestring,
             'operating_systems': basestring,
             'record_into_store': basestring,
         }
@@ -174,6 +176,26 @@ class ScalarType:
                             .format(field, self._name, LIST_FIELDS_CONTENT[field].__name__,
                                     BASE_DOC_URL)).handle_later()
 
+        # Check that keys are only added to keyed scalars and that their values are valid
+        MAX_KEY_COUNT = 100
+        MAX_KEY_LENGTH = 72
+        keys = definition.get('keys')
+        if keys is not None:
+            if not definition.get('keyed', False):
+                ParserError(self._name + '- invalid field: ' +
+                            '\n`keys` field only valid for keyed histograms').handle_later()
+
+            if len(keys) > MAX_KEY_COUNT:
+                ParserError(self._name + ' - exceeding key count: ' +
+                            '\n`keys` values count  must not exceed {}'.format(MAX_KEY_COUNT))\
+                            .handle_later()
+
+            invalid = filter(lambda k: len(k) > MAX_KEY_LENGTH, keys)
+            if len(invalid) > 0:
+                ParserError(self._name + ' - invalid key value' +
+                            '\n `keys` values are exceeding length {}:'.format(MAX_LENGTH_COUNT) +
+                            ', '.join(invalid)).handle_later()
+
     def validate_values(self, definition):
         """This function checks that the fields have the correct values.
 
@@ -277,6 +299,11 @@ class ScalarType:
         """Get the scalar kind"""
         return self._definition['kind']
 
+    @property
+    def keys(self):
+        """Get the allowed keys for this scalar or [] if there aren't any'"""
+        return self._definition.get('keys', [])
+
     @property
     def keyed(self):
         """Boolean indicating whether this is a keyed scalar"""

toolkit/components/telemetry/core/ScalarInfo.h

@@ -23,18 +23,23 @@ struct BaseScalarInfo {
   uint32_t dataset;
   mozilla::Telemetry::Common::RecordedProcessType record_in_processes;
   bool keyed;
+  uint32_t key_count;
+  uint32_t key_offset;
   mozilla::Telemetry::Common::SupportedProduct products;
   bool builtin;
 
   constexpr BaseScalarInfo(
       uint32_t aKind, uint32_t aDataset,
       mozilla::Telemetry::Common::RecordedProcessType aRecordInProcess,
-      bool aKeyed, mozilla::Telemetry::Common::SupportedProduct aProducts,
+      bool aKeyed, uint32_t aKeyCount, uint32_t aKeyOffset,
+      mozilla::Telemetry::Common::SupportedProduct aProducts,
       bool aBuiltin = true)
       : kind(aKind),
         dataset(aDataset),
         record_in_processes(aRecordInProcess),
         keyed(aKeyed),
+        key_count(aKeyCount),
+        key_offset(aKeyOffset),
         products(aProducts),
         builtin(aBuiltin) {}
   virtual ~BaseScalarInfo() {}
@@ -43,7 +48,6 @@ struct BaseScalarInfo {
   virtual const char* expiration() const = 0;
 
   virtual uint32_t storeOffset() const = 0;
-
   virtual uint32_t storeCount() const = 0;
 };
 
@@ -68,9 +72,11 @@ struct ScalarInfo : BaseScalarInfo {
       uint32_t aKind, uint32_t aNameOffset, uint32_t aExpirationOffset,
       uint32_t aDataset,
       mozilla::Telemetry::Common::RecordedProcessType aRecordInProcess,
-      bool aKeyed, mozilla::Telemetry::Common::SupportedProduct aProducts,
-      uint32_t aStoreCount, uint16_t aStoreOffset)
-      : BaseScalarInfo(aKind, aDataset, aRecordInProcess, aKeyed, aProducts),
+      bool aKeyed, uint32_t aKeyCount, uint32_t aKeyOffset,
+      mozilla::Telemetry::Common::SupportedProduct aProducts,
+      uint32_t aStoreCount, uint32_t aStoreOffset)
+      : BaseScalarInfo(aKind, aDataset, aRecordInProcess, aKeyed, aKeyCount,
+                       aKeyOffset, aProducts),
         name_offset(aNameOffset),
         expiration_offset(aExpirationOffset),
         store_count(aStoreCount),
@@ -80,7 +86,6 @@ struct ScalarInfo : BaseScalarInfo {
   const char* expiration() const override;
 
   uint32_t storeOffset() const override { return store_offset; };
-
   uint32_t storeCount() const override { return store_count; };
 };
 

toolkit/components/telemetry/core/TelemetryScalar.cpp

@@ -140,6 +140,7 @@ enum class ScalarResult : uint8_t {
   KeyIsEmpty,
   KeyTooLong,
   TooManyKeys,
+  KeyNotAllowed,
   // String Scalar Errors
   StringTooLong,
   // Unsigned Scalar Errors
@@ -168,11 +169,12 @@ struct DynamicScalarInfo : BaseScalarInfo {
   DynamicScalarInfo(uint32_t aKind, bool aRecordOnRelease, bool aExpired,
                     const nsACString& aName, bool aKeyed, bool aBuiltin,
                     const nsTArray<nsCString>& aStores)
-      : BaseScalarInfo(
-            aKind,
-            aRecordOnRelease ? nsITelemetry::DATASET_ALL_CHANNELS
-                             : nsITelemetry::DATASET_PRERELEASE_CHANNELS,
-            RecordedProcessType::All, aKeyed, GetCurrentProduct(), aBuiltin),
+      : BaseScalarInfo(aKind,
+                       aRecordOnRelease
+                           ? nsITelemetry::DATASET_ALL_CHANNELS
+                           : nsITelemetry::DATASET_PRERELEASE_CHANNELS,
+                       RecordedProcessType::All, aKeyed, 0, 0,
+                       GetCurrentProduct(), aBuiltin),
         mDynamicName(aName),
         mDynamicExpiration(aExpired) {
     store_count = aStores.Length();
@@ -210,7 +212,6 @@ const char* DynamicScalarInfo::expiration() const {
 }
 
 uint32_t DynamicScalarInfo::storeOffset() const { return store_offset; }
-
 uint32_t DynamicScalarInfo::storeCount() const { return store_count; }
 
 typedef nsBaseHashtableET<nsDepCharHashKey, ScalarKey> CharPtrEntryType;
@@ -851,7 +852,10 @@ class KeyedScalar {
   // We store the name instead of a reference to the BaseScalarInfo because
   // the BaseScalarInfo can move if it's from a dynamic scalar.
   explicit KeyedScalar(const BaseScalarInfo& info)
-      : mScalarName(info.name()), mMaximumNumberOfKeys(kMaximumNumberOfKeys){};
+      : mScalarName(info.name()),
+        mScalarKeyCount(info.key_count),
+        mScalarKeyOffset(info.key_offset),
+        mMaximumNumberOfKeys(kMaximumNumberOfKeys){};
   ~KeyedScalar() = default;
 
   // Set, Add and SetMaximum functions as described in the Telemetry IDL.
@@ -891,10 +895,14 @@ class KeyedScalar {
 
   const nsCString mScalarName;
   ScalarKeysMapType mScalarKeys;
+  uint32_t mScalarKeyCount;
+  uint32_t mScalarKeyOffset;
   uint32_t mMaximumNumberOfKeys;
 
   ScalarResult GetScalarForKey(const StaticMutexAutoLock& locker,
                                const nsAString& aKey, ScalarBase** aRet);
+
+  bool AllowsKey(const nsAString& aKey) const;
 };
 
 ScalarResult KeyedScalar::SetValue(const StaticMutexAutoLock& locker,
@@ -939,7 +947,7 @@ void KeyedScalar::SetValue(const StaticMutexAutoLock& locker,
   if (sr != ScalarResult::Ok) {
     // Bug 1451813 - We now report which scalars exceed the key limit in
     // telemetry.keyed_scalars_exceed_limit.
-    if (sr != ScalarResult::TooManyKeys) {
+    if (sr == ScalarResult::KeyTooLong) {
       MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
     }
     return;
@@ -956,7 +964,7 @@ void KeyedScalar::SetValue(const StaticMutexAutoLock& locker,
   if (sr != ScalarResult::Ok) {
     // Bug 1451813 - We now report which scalars exceed the key limit in
     // telemetry.keyed_scalars_exceed_limit.
-    if (sr != ScalarResult::TooManyKeys) {
+    if (sr == ScalarResult::KeyTooLong) {
       MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
     }
     return;
@@ -973,7 +981,7 @@ void KeyedScalar::AddValue(const StaticMutexAutoLock& locker,
   if (sr != ScalarResult::Ok) {
     // Bug 1451813 - We now report which scalars exceed the key limit in
     // telemetry.keyed_scalars_exceed_limit.
-    if (sr != ScalarResult::TooManyKeys) {
+    if (sr == ScalarResult::KeyTooLong) {
       MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
     }
     return;
@@ -990,9 +998,10 @@ void KeyedScalar::SetMaximum(const StaticMutexAutoLock& locker,
   if (sr != ScalarResult::Ok) {
     // Bug 1451813 - We now report which scalars exceed the key limit in
     // telemetry.keyed_scalars_exceed_limit.
-    if (sr != ScalarResult::TooManyKeys) {
+    if (sr == ScalarResult::KeyTooLong) {
       MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
     }
+
     return;
   }
 
@@ -1052,6 +1061,23 @@ ScalarResult KeyedScalar::GetScalarForKey(const StaticMutexAutoLock& locker,
     return ScalarResult::KeyIsEmpty;
   }
 
+  if (!AllowsKey(aKey)) {
+    KeyedScalar* scalarUnknown = nullptr;
+    ScalarKey scalarUnknownUniqueId{
+        static_cast<uint32_t>(
+            mozilla::Telemetry::ScalarID::TELEMETRY_KEYED_SCALARS_UNKNOWN_KEYS),
+        false};
+    ProcessID process = ProcessID::Parent;
+    nsresult rv = internal_GetKeyedScalarByEnum(locker, scalarUnknownUniqueId,
+                                                process, &scalarUnknown);
+    if (NS_FAILED(rv)) {
+      return ScalarResult::TooManyKeys;
+    }
+    scalarUnknown->AddValue(locker, NS_ConvertUTF8toUTF16(mScalarName), 1);
+
+    return ScalarResult::KeyNotAllowed;
+  }
+
   if (aKey.Length() > kMaximumKeyStringLength) {
     return ScalarResult::KeyTooLong;
   }
@@ -1117,6 +1143,22 @@ size_t KeyedScalar::SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) {
   return n;
 }
 
+bool KeyedScalar::AllowsKey(const nsAString& aKey) const {
+  // If we didn't specify a list of allowed keys, just return true.
+  if (mScalarKeyCount == 0) {
+    return true;
+  }
+
+  for (uint32_t i = 0; i < mScalarKeyCount; ++i) {
+    uint32_t stringIndex = gScalarKeysTable[mScalarKeyOffset + i];
+    if (aKey.EqualsASCII(&gScalarsStringTable[stringIndex])) {
+      return true;
+    }
+  }
+
+  return false;
+}
+
 typedef nsUint32HashKey ScalarIDHashKey;
 typedef nsUint32HashKey ProcessIDHashKey;
 typedef nsClassHashtable<ScalarIDHashKey, ScalarBase> ScalarStorageMapType;
@@ -1257,6 +1299,11 @@ void internal_LogScalarError(const nsACString& aScalarName, ScalarResult aSr) {
                           kMaximumKeyStringLength),
           errorMessage);
       break;
+    case ScalarResult::KeyNotAllowed:
+      AppendUTF8toUTF16(
+          nsPrintfCString(" - The key is not allowed for this scalar."),
+          errorMessage);
+      break;
     case ScalarResult::TooManyKeys:
       AppendUTF8toUTF16(
           nsPrintfCString(" - Keyed scalars cannot have more than %d keys.",

toolkit/components/telemetry/docs/collection/scalars.rst

@@ -178,6 +178,7 @@ Optional Fields
 
 - ``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, unless the user has opted out. With the latter the scalar is submitted by default on release and pre-release channels, unless the user has opted out.
 - ``keyed``: A boolean that determines whether this is a keyed scalar. It defaults to ``false``.
+- ``keys``: A string list. Only valid for *keyed scalars*. Defines a case insensititve list of allowed keys that can be used for this scalar. The list is limited to 100 keys with a maximum length of 72 characters each. When using a key that is not in the list, an error is returned.
 - ``record_into_store``: A list of stores this scalar should be recorded into. It defaults to ``[main]``.
 - ``operating_systems``: This field restricts recording to certain operating systems only. Use that in-place of previous ``cpp_guards`` to avoid inclusion on not-specified operating systems. It defaults to ``all``. Currently supported values are:
 

toolkit/components/telemetry/tests/gtest/TestScalars.cpp

@@ -419,3 +419,42 @@ TEST_F(TelemetryTestFixture, WrongKeyedScalarOperator) {
   CheckKeyedBoolScalar("telemetry.test.keyed_boolean_kind", "key2",
                        cx.GetJSContext(), scalarsSnapshot, true);
 }
+
+TEST_F(TelemetryTestFixture, TestKeyedScalarAllowedKeys) {
+  AutoJSContextWithGlobal cx(mCleanGlobal);
+  // Make sure we don't get scalars from other tests.
+  Unused << mTelemetry->ClearScalars();
+
+  const uint32_t kExpectedUint = 1172017;
+
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("only"), kExpectedUint);
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("meant"), kExpectedUint);
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("for"), kExpectedUint);
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("testing"), kExpectedUint);
+
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("invalid"), kExpectedUint);
+  Telemetry::ScalarSet(Telemetry::ScalarID::TELEMETRY_TEST_KEYED_WITH_KEYS,
+                       NS_LITERAL_STRING("not-valid"), kExpectedUint);
+
+  JS::RootedValue scalarsSnapshot(cx.GetJSContext());
+  GetScalarsSnapshot(true, cx.GetJSContext(), &scalarsSnapshot);
+  CheckKeyedUintScalar("telemetry.test.keyed_with_keys", "only",
+                       cx.GetJSContext(), scalarsSnapshot, kExpectedUint);
+  CheckKeyedUintScalar("telemetry.test.keyed_with_keys", "meant",
+                       cx.GetJSContext(), scalarsSnapshot, kExpectedUint);
+  CheckKeyedUintScalar("telemetry.test.keyed_with_keys", "for",
+                       cx.GetJSContext(), scalarsSnapshot, kExpectedUint);
+  CheckKeyedUintScalar("telemetry.test.keyed_with_keys", "testing",
+                       cx.GetJSContext(), scalarsSnapshot, kExpectedUint);
+  CheckNumberOfProperties("telemetry.test.keyed_with_keys", cx.GetJSContext(),
+                          scalarsSnapshot, 4);
+
+  CheckKeyedUintScalar("telemetry.keyed_scalars_unknown_keys",
+                       "telemetry.test.keyed_with_keys", cx.GetJSContext(),
+                       scalarsSnapshot, 2);
+}

toolkit/components/telemetry/tests/python/test_gen_scalar_data_json.py

@@ -58,6 +58,7 @@ newscalar:
                     "expires": "never",
                     "record_on_release": True,
                     "keyed": False,
+                    "keys": [],
                     "stores": ["main"],
                     "products": ["firefox", "fennec", "geckoview"],
                 },
@@ -67,6 +68,7 @@ newscalar:
                     "expires": "never",
                     "record_on_release": False,
                     "keyed": False,
+                    "keys": [],
                     "stores": ["main"],
                     "products": ["firefox"],
                 }