Bug 1451813 - Report which keyed scalars fail to accumulate due to running out of keys. r=chutten
authorNicklas Boman <smurfd@gmail.com>
Fri, 12 Oct 2018 23:03:49 +0200
changeset 491103 f54dd68a3330bc25ff9896a0dcc08cc69d19d936
parent 491102 0a7e1b194b75e6d7c556c98ac7a1a68b948bb076
child 491104 14c0adad8a503f183a52ae35466da67c1f72f186
push id247
push userfmarier@mozilla.com
push dateSat, 27 Oct 2018 01:06:44 +0000
reviewerschutten
bugs1451813
milestone65.0a1
Bug 1451813 - Report which keyed scalars fail to accumulate due to running out of keys. r=chutten
toolkit/components/telemetry/Scalars.yaml
toolkit/components/telemetry/core/TelemetryScalar.cpp
toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
--- a/toolkit/components/telemetry/Scalars.yaml
+++ b/toolkit/components/telemetry/Scalars.yaml
@@ -1703,16 +1703,30 @@ telemetry:
     kind: boolean
     notification_emails:
       - jrediger@mozilla.com
       - telemetry-client-dev@mozilla.com
     release_channel_collection: opt-out
     record_in_processes:
       - 'main'
 
+  keyed_scalars_exceed_limit:
+    bug_numbers:
+      - 1451813
+    description: >
+      The number of times keyed scalars exceeded the number of keys limit, keyed by scalar name.
+    expires: "never"
+    kind: uint
+    keyed: true
+    notification_emails:
+      - chutten@mozilla.com
+    release_channel_collection: opt-out
+    record_in_processes:
+      - 'main'
+
 telemetry.discarded:
   accumulations:
     bug_numbers:
       - 1369041
     description: >
       Number of discarded accumulations to histograms in child processes
     expires: "never"
     kind: uint
--- a/toolkit/components/telemetry/core/TelemetryScalar.cpp
+++ b/toolkit/components/telemetry/core/TelemetryScalar.cpp
@@ -682,33 +682,33 @@ internal_ScalarAllocate(uint32_t aScalar
 /**
  * The implementation for the keyed scalar type.
  */
 class KeyedScalar
 {
 public:
   typedef mozilla::Pair<nsCString, nsCOMPtr<nsIVariant>> KeyValuePair;
 
-  explicit KeyedScalar(uint32_t aScalarKind)
-    : mScalarKind(aScalarKind)
+  explicit KeyedScalar(const BaseScalarInfo& info)
+    : mScalarInfo(info)
     , mMaximumNumberOfKeys(kMaximumNumberOfKeys)
   { };
   ~KeyedScalar() = default;
 
   // Set, Add and SetMaximum functions as described in the Telemetry IDL.
   // These methods implicitly instantiate a Scalar[*] for each key.
-  ScalarResult SetValue(const nsAString& aKey, nsIVariant* aValue);
-  ScalarResult AddValue(const nsAString& aKey, nsIVariant* aValue);
-  ScalarResult SetMaximum(const nsAString& aKey, nsIVariant* aValue);
+  ScalarResult SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue);
+  ScalarResult AddValue(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue);
+  ScalarResult SetMaximum(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue);
 
   // Convenience methods used by the C++ API.
-  void SetValue(const nsAString& aKey, uint32_t aValue);
-  void SetValue(const nsAString& aKey, bool aValue);
-  void AddValue(const nsAString& aKey, uint32_t aValue);
-  void SetMaximum(const nsAString& aKey, uint32_t aValue);
+  void SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, uint32_t aValue);
+  void SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, bool aValue);
+  void AddValue(const StaticMutexAutoLock& locker, const nsAString& aKey, uint32_t aValue);
+  void SetMaximum(const StaticMutexAutoLock& locker, 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;
 
   // To measure the memory stats.
   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf);
 
@@ -717,104 +717,120 @@ public:
   {
     mMaximumNumberOfKeys = aMaximumNumberOfKeys;
   };
 
 private:
   typedef nsClassHashtable<nsCStringHashKey, ScalarBase> ScalarKeysMapType;
 
   ScalarKeysMapType mScalarKeys;
-  const uint32_t mScalarKind;
+  const BaseScalarInfo& mScalarInfo;
   uint32_t mMaximumNumberOfKeys;
 
-  ScalarResult GetScalarForKey(const nsAString& aKey, ScalarBase** aRet);
+  ScalarResult GetScalarForKey(const StaticMutexAutoLock& locker, const nsAString& aKey, ScalarBase** aRet);
 };
 
 ScalarResult
-KeyedScalar::SetValue(const nsAString& aKey, nsIVariant* aValue)
+KeyedScalar::SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
   if (sr != ScalarResult::Ok) {
     return sr;
   }
 
   return scalar->SetValue(aValue);
 }
 
 ScalarResult
-KeyedScalar::AddValue(const nsAString& aKey, nsIVariant* aValue)
+KeyedScalar::AddValue(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
   if (sr != ScalarResult::Ok) {
     return sr;
   }
 
   return scalar->AddValue(aValue);
 }
 
 ScalarResult
-KeyedScalar::SetMaximum(const nsAString& aKey, nsIVariant* aValue)
+KeyedScalar::SetMaximum(const StaticMutexAutoLock& locker, const nsAString& aKey, nsIVariant* aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
   if (sr != ScalarResult::Ok) {
     return sr;
   }
 
   return scalar->SetMaximum(aValue);
 }
 
 void
-KeyedScalar::SetValue(const nsAString& aKey, uint32_t aValue)
+KeyedScalar::SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, uint32_t aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
+
   if (sr != ScalarResult::Ok) {
-    MOZ_ASSERT(false, "Key too long or too many keys are recorded in the scalar.");
+    // Bug 1451813 - We now report which scalars exceed the key limit in telemetry.keyed_scalars_exceed_limit.
+    if(sr != ScalarResult::TooManyKeys) {
+      MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
+    }
     return;
   }
 
   return scalar->SetValue(aValue);
 }
 
 void
-KeyedScalar::SetValue(const nsAString& aKey, bool aValue)
+KeyedScalar::SetValue(const StaticMutexAutoLock& locker, const nsAString& aKey, bool aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
+
   if (sr != ScalarResult::Ok) {
-    MOZ_ASSERT(false, "Key too long or too many keys are recorded in the scalar.");
+    // Bug 1451813 - We now report which scalars exceed the key limit in telemetry.keyed_scalars_exceed_limit.
+    if (sr != ScalarResult::TooManyKeys) {
+      MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
+    }
     return;
   }
 
   return scalar->SetValue(aValue);
 }
 
 void
-KeyedScalar::AddValue(const nsAString& aKey, uint32_t aValue)
+KeyedScalar::AddValue(const StaticMutexAutoLock& locker, const nsAString& aKey, uint32_t aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
+
   if (sr != ScalarResult::Ok) {
-    MOZ_ASSERT(false, "Key too long or too many keys are recorded in the scalar.");
+    // Bug 1451813 - We now report which scalars exceed the key limit in telemetry.keyed_scalars_exceed_limit.
+    if (sr != ScalarResult::TooManyKeys) {
+      MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
+    }
     return;
   }
 
   return scalar->AddValue(aValue);
 }
 
 void
-KeyedScalar::SetMaximum(const nsAString& aKey, uint32_t aValue)
+KeyedScalar::SetMaximum(const StaticMutexAutoLock& locker, const nsAString& aKey, uint32_t aValue)
 {
   ScalarBase* scalar = nullptr;
-  ScalarResult sr = GetScalarForKey(aKey, &scalar);
+  ScalarResult sr = GetScalarForKey(locker, aKey, &scalar);
+
   if (sr != ScalarResult::Ok) {
-    MOZ_ASSERT(false, "Key too long or too many keys are recorded in the scalar.");
+    // Bug 1451813 - We now report which scalars exceed the key limit in telemetry.keyed_scalars_exceed_limit.
+    if(sr != ScalarResult::TooManyKeys) {
+      MOZ_ASSERT(false, "Key too long to be recorded in the scalar.");
+    }
     return;
   }
 
   return scalar->SetMaximum(aValue);
 }
 
 /**
  * Get a key-value array with the values for the Keyed Scalar.
@@ -838,23 +854,30 @@ KeyedScalar::GetValue(nsTArray<KeyValueP
 
     // Append it to value list.
     aValues.AppendElement(mozilla::MakePair(nsCString(iter.Key()), scalarValue));
   }
 
   return NS_OK;
 }
 
+// Forward declaration
+nsresult
+internal_GetKeyedScalarByEnum(const StaticMutexAutoLock& lock,
+                              const ScalarKey& aId,
+                              ProcessID aProcessStorage,
+                              KeyedScalar** aRet);
+
 /**
  * Get the scalar for the referenced key.
  * If there's no such key, instantiate a new Scalar object with the
  * same type of the Keyed scalar and create the key.
  */
 ScalarResult
-KeyedScalar::GetScalarForKey(const nsAString& aKey, ScalarBase** aRet)
+KeyedScalar::GetScalarForKey(const StaticMutexAutoLock& locker, const nsAString& aKey, ScalarBase** aRet)
 {
   if (aKey.IsEmpty()) {
     return ScalarResult::KeyIsEmpty;
   }
 
   if (aKey.Length() > kMaximumKeyStringLength) {
     return ScalarResult::KeyTooLong;
   }
@@ -863,20 +886,37 @@ KeyedScalar::GetScalarForKey(const nsASt
 
   ScalarBase* scalar = nullptr;
   if (mScalarKeys.Get(utf8Key, &scalar)) {
     *aRet = scalar;
     return ScalarResult::Ok;
   }
 
   if (mScalarKeys.Count() >= mMaximumNumberOfKeys) {
+    if(aKey.EqualsLiteral("telemetry.keyed_scalars_exceed_limit")) {
+      return ScalarResult::TooManyKeys;
+    }
+
+    KeyedScalar* scalarExceed = nullptr;
+
+    ScalarKey uniqueId{static_cast<uint32_t>(mozilla::Telemetry::ScalarID::TELEMETRY_KEYED_SCALARS_EXCEED_LIMIT), false};
+
+    ProcessID process = ProcessID::Parent;
+    nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, process, &scalarExceed);
+
+    if (NS_FAILED(rv)) {
+      return ScalarResult::TooManyKeys;
+    }
+
+    scalarExceed->AddValue(locker, NS_ConvertUTF8toUTF16(mScalarInfo.name()), 1);
+
     return ScalarResult::TooManyKeys;
   }
 
-  scalar = internal_ScalarAllocate(mScalarKind);
+  scalar = internal_ScalarAllocate(mScalarInfo.kind);
   if (!scalar) {
     return ScalarResult::InvalidType;
   }
 
   mScalarKeys.Put(utf8Key, scalar);
 
   *aRet = scalar;
   return ScalarResult::Ok;
@@ -1565,17 +1605,17 @@ internal_GetKeyedScalarByEnum(const Stat
   }
 
   // We don't currently support keyed string scalars. Disable them.
   if (info.kind == nsITelemetry::SCALAR_TYPE_STRING) {
     MOZ_ASSERT(false, "Keyed string scalars are not currently supported.");
     return NS_ERROR_INVALID_ARG;
   }
 
-  scalar = new KeyedScalar(info.kind);
+  scalar = new KeyedScalar(info);
   if (!scalar) {
     return NS_ERROR_INVALID_ARG;
   }
 
   scalarStorage->Put(aId.id, scalar);
   *aRet = scalar;
   return NS_OK;
 }
@@ -1652,23 +1692,23 @@ internal_UpdateKeyedScalar(const StaticM
     // Don't throw on expired scalars.
     if (rv == NS_ERROR_NOT_AVAILABLE) {
       return ScalarResult::Ok;
     }
     return ScalarResult::UnknownScalar;
   }
 
   if (aType == ScalarActionType::eAdd) {
-    return scalar->AddValue(aKey, aValue);
+    return scalar->AddValue(lock, aKey, aValue);
   }
   if (aType == ScalarActionType::eSet) {
-    return scalar->SetValue(aKey, aValue);
+    return scalar->SetValue(lock, aKey, aValue);
   }
 
-  return scalar->SetMaximum(aKey, aValue);
+  return scalar->SetMaximum(lock, aKey, aValue);
 }
 
 /**
  * Helper function to convert an array of |DynamicScalarInfo|
  * to |DynamicScalarDefinition| used by the IPC calls.
  */
 void
 internal_DynamicScalarToIPC(const StaticMutexAutoLock& lock,
@@ -2104,24 +2144,24 @@ internal_ApplyKeyedScalarActions(const S
         {
           switch (scalarType)
           {
             case nsITelemetry::SCALAR_TYPE_COUNT:
               if (!upd.mData->is<uint32_t>()) {
                 NS_WARNING("Attempting to set a count scalar to a non-integer.");
                 continue;
               }
-              scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
+              scalar->SetValue(lock, NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
               break;
             case nsITelemetry::SCALAR_TYPE_BOOLEAN:
               if (!upd.mData->is<bool>()) {
                 NS_WARNING("Attempting to set a boolean scalar to a non-boolean.");
                 continue;
               }
-              scalar->SetValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<bool>());
+              scalar->SetValue(lock, NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<bool>());
               break;
             default:
               NS_WARNING("Unsupported type coming from scalar child updates.");
           }
           break;
         }
       case ScalarActionType::eAdd:
         {
@@ -2129,31 +2169,31 @@ internal_ApplyKeyedScalarActions(const S
             NS_WARNING("Attempting to add on a non count scalar.");
             continue;
           }
           // We only support adding on uint32_t.
           if (!upd.mData->is<uint32_t>()) {
             NS_WARNING("Attempting to add to a count scalar with a non-integer.");
             continue;
           }
-          scalar->AddValue(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
+          scalar->AddValue(lock, NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
           break;
         }
       case ScalarActionType::eSetMaximum:
         {
           if (scalarType != nsITelemetry::SCALAR_TYPE_COUNT) {
             NS_WARNING("Attempting to setMaximum on a non count scalar.");
             continue;
           }
           // We only support SetMaximum on uint32_t.
           if (!upd.mData->is<uint32_t>()) {
             NS_WARNING("Attempting to setMaximum a count scalar to a non-integer.");
             continue;
           }
-          scalar->SetMaximum(NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
+          scalar->SetMaximum(lock, NS_ConvertUTF8toUTF16(upd.mKey), upd.mData->as<uint32_t>());
           break;
         }
       default:
         NS_WARNING("Unsupported action coming from keyed scalar child updates.");
     }
   }
 }
 
@@ -2433,17 +2473,17 @@ TelemetryScalar::Add(mozilla::Telemetry:
   KeyedScalar* scalar = nullptr;
   nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId,
                                               ProcessID::Parent,
                                               &scalar);
   if (NS_FAILED(rv)) {
     return;
   }
 
-  scalar->AddValue(aKey, aValue);
+  scalar->AddValue(locker, aKey, aValue);
 }
 
 /**
  * Sets the scalar to the given value.
  *
  * @param aName The scalar name.
  * @param aVal The value to set the scalar to.
  * @param aCx The JS context.
@@ -2699,17 +2739,17 @@ TelemetryScalar::Set(mozilla::Telemetry:
   KeyedScalar* scalar = nullptr;
   nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId,
                                               ProcessID::Parent,
                                               &scalar);
   if (NS_FAILED(rv)) {
     return;
   }
 
-  scalar->SetValue(aKey, aValue);
+  scalar->SetValue(locker, aKey, aValue);
 }
 
 /**
  * Sets the scalar to the given boolean value.
  *
  * @param aId The scalar enum id.
  * @param aKey The scalar key.
  * @param aValue The boolean value to set the scalar to.
@@ -2749,17 +2789,17 @@ TelemetryScalar::Set(mozilla::Telemetry:
   KeyedScalar* scalar = nullptr;
   nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId,
                                               ProcessID::Parent,
                                               &scalar);
   if (NS_FAILED(rv)) {
     return;
   }
 
-  scalar->SetValue(aKey, aValue);
+  scalar->SetValue(locker, aKey, aValue);
 }
 
 /**
  * Sets the scalar to the maximum of the current and the passed value.
  *
  * @param aName The scalar name.
  * @param aVal The numeric value to set the scalar to.
  * @param aCx The JS context.
@@ -2920,17 +2960,17 @@ TelemetryScalar::SetMaximum(mozilla::Tel
 
   KeyedScalar* scalar = nullptr;
   nsresult rv = internal_GetKeyedScalarByEnum(locker, uniqueId, ProcessID::Parent,
                                               &scalar);
   if (NS_FAILED(rv)) {
     return;
   }
 
-  scalar->SetMaximum(aKey, aValue);
+  scalar->SetMaximum(locker, aKey, aValue);
 }
 
 /**
  * Serializes the scalars from the given dataset to a json-style object and resets them.
  * The returned structure looks like:
  *    {"process": {"category1.probe":1,"category1.other_probe":false,...}, ... }.
  *
  * @param aDataset DATASET_RELEASE_CHANNEL_OPTOUT or DATASET_RELEASE_CHANNEL_OPTIN.
@@ -3257,17 +3297,17 @@ TelemetryScalar::SummarizeEvent(const ns
   }
 
   static uint32_t sMaxEventSummaryKeys =
     Preferences::GetUint("toolkit.telemetry.maxEventSummaryKeys", 500);
 
   // Set this each time as it may have been cleared and recreated between calls
   scalar->SetMaximumNumberOfKeys(sMaxEventSummaryKeys);
 
-  scalar->AddValue(NS_ConvertASCIItoUTF16(aUniqueEventName), 1);
+  scalar->AddValue(lock, NS_ConvertASCIItoUTF16(aUniqueEventName), 1);
 }
 
 /**
  * Resets all the stored scalars. This is intended to be only used in tests.
  */
 void
 TelemetryScalar::ClearScalars()
 {
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryScalars.js
@@ -1,16 +1,17 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/
 */
 
 const UINT_SCALAR = "telemetry.test.unsigned_int_kind";
 const STRING_SCALAR = "telemetry.test.string_kind";
 const BOOLEAN_SCALAR = "telemetry.test.boolean_kind";
 const KEYED_UINT_SCALAR = "telemetry.test.keyed_unsigned_int";
+const KEYED_EXCEED_SCALAR = "telemetry.keyed_scalars_exceed_limit";
 
 function getProcessScalars(aChannel, aProcessName, aKeyed = false, aClear = false) {
   const scalars = aKeyed ?
     Telemetry.snapshotKeyedScalars(aChannel, aClear)[aProcessName] :
     Telemetry.snapshotScalars(aChannel, aClear)[aProcessName];
   return scalars || {};
 }
 
@@ -529,16 +530,41 @@ add_task(async function test_keyed_max_k
             "The keyed scalar must contain all the 100 keys, and drop the others.");
 
   // Check that all the keys recorded the expected values.
   let expectedValue = 0;
   keyNamesSet.forEach(keyName => {
     Assert.equal(keyedScalars[KEYED_UINT_SCALAR][keyName], expectedValue++,
                  "The key must contain the expected value.");
   });
+
+  // Check that KEYED_EXCEED_SCALAR is in keyedScalars
+  Assert.ok((KEYED_EXCEED_SCALAR in keyedScalars),
+            "We have exceeded maximum number of Keys.");
+
+  // Generate the names for the exceeded keys
+  let keyNamesSet2 = new Set();
+  for (let k = 0; k < 100; k++) {
+    keyNamesSet2.add("key2_" + k);
+  }
+
+  // Add 100 keys to the keyed exceed scalar and set their initial value.
+  valueToSet = 0;
+  keyNamesSet2.forEach(keyName2 => {
+    Telemetry.keyedScalarSet(KEYED_EXCEED_SCALAR, keyName2, valueToSet++);
+  });
+
+  // Check that there are exactly 100 keys in KEYED_EXCEED_SCALAR
+  let snapshot = Telemetry.snapshotKeyedScalars(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false);
+  Assert.equal(100, Object.keys(snapshot.parent[KEYED_UINT_SCALAR]).length,
+             "The keyed scalar must contain all the 100 keys.");
+
+  // Check that KEYED_UINT_SCALAR is in keyedScalars and its value equals 3
+  Assert.ok((KEYED_UINT_SCALAR in keyedScalars[KEYED_EXCEED_SCALAR]), "The keyed Scalar is in the keyed exceeded scalar");
+  Assert.equal(keyedScalars[KEYED_EXCEED_SCALAR][KEYED_UINT_SCALAR], 3, "We have exactly 3 keys over the limit");
 });
 
 add_task(async function test_dynamicScalars_registration() {
   Telemetry.clearScalars();
 
   const TEST_CASES = [
     {
       "category": "telemetry.test",