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 442833 f54dd68a3330bc25ff9896a0dcc08cc69d19d936
parent 442832 0a7e1b194b75e6d7c556c98ac7a1a68b948bb076
child 442834 14c0adad8a503f183a52ae35466da67c1f72f186
push id34925
push userrgurzau@mozilla.com
push dateWed, 24 Oct 2018 22:00:55 +0000
treeherdermozilla-central@ddadc29de671 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerschutten
bugs1451813
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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",