bug 1366294 - Part 10 - Nail down count histogram semantics. r?gfritzsche draft
authorChris H-C <chutten@mozilla.com>
Tue, 04 Jul 2017 10:12:23 -0400
changeset 609067 088deab83a17430b67ef6970341f3741ec8734b8
parent 609066 34bba71d45c896bbaf4cf73a960fea773a2062cf
child 609068 70c09f5fa09175063d38c5733e82f4c4ce6585ae
push id68499
push userbmo:chutten@mozilla.com
push dateFri, 14 Jul 2017 19:04:19 +0000
reviewersgfritzsche
bugs1366294
milestone56.0a1
bug 1366294 - Part 10 - Nail down count histogram semantics. r?gfritzsche Previously we assumed count histograms were always present in payloads. This was an erroneous assumption as count histograms were only 0 if they were session histograms, or if they were from subsession histograms from subsessions _after_ a subsession when they held a non-0 value. So let's just treat count histograms as normal histograms from now on, without any of this "sometimes 0" nonsense. This simplifies the code, tests, and our understanding... and _should_ have few/zero downstream effects since the existing behaviour was so poorly-understood (though exactly tested). MozReview-Commit-ID: BH108ksygGw
toolkit/components/telemetry/TelemetryHistogram.cpp
toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -606,22 +606,22 @@ internal_ReflectHistogramSnapshot(JSCont
   Histogram::SampleSet ss;
   h->SnapshotSample(&ss);
   return internal_ReflectHistogramAndSamples(cx, obj, h, ss);
 }
 
 bool
 internal_ShouldReflectHistogram(Histogram* h, HistogramID id)
 {
-  // Only count & flag histograms are serialized when they are empty.
+  // Only flag histograms are serialized when they are empty.
   // This has historical reasons, changing this will require downstream changes.
   // The cheaper path here is to just deprecate flag histograms in favor
   // of scalars.
   uint32_t type = gHistogramInfos[id].histogramType;
-  if (internal_IsEmpty(h) && (type != nsITelemetry::HISTOGRAM_FLAG)) {
+  if (internal_IsEmpty(h) && type != nsITelemetry::HISTOGRAM_FLAG) {
     return false;
   }
 
   return true;
 }
 
 } // namespace
 
@@ -951,27 +951,22 @@ internal_ClearHistogram(HistogramID id, 
   nsTArray<SessionType> sessionTypes;
   if (!onlySubsession) {
     sessionTypes.AppendElement(SessionType::Session);
   }
 #if !defined(MOZ_WIDGET_ANDROID)
   sessionTypes.AppendElement(SessionType::Subsession);
 #endif
 
-  if (sessionTypes.Length() == 0) {
-    // Nothing to do here.
-    return;
-  }
-
   // Now reset the histograms instances for all processes.
-  for (uint32_t sessionIdx = 0; sessionIdx < sessionTypes.Length(); ++sessionIdx) {
+  for (SessionType sessionType : sessionTypes) {
     for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
       internal_ClearHistogramById(id,
                                   static_cast<ProcessID>(process),
-                                  sessionTypes[sessionIdx]);
+                                  sessionType);
     }
   }
 }
 
 } // namespace
 
 
 ////////////////////////////////////////////////////////////////////////
@@ -1918,38 +1913,16 @@ TelemetryHistogram::CreateHistogramSnaps
   }
 
 #if !defined(MOZ_WIDGET_ANDROID)
   SessionType sessionType = SessionType(subsession);
 #else
   SessionType sessionType = SessionType::Session;
 #endif
 
-  // Ensure that all the HISTOGRAM_FLAG histograms have
-  // been created, so that their values are snapshotted.
-  auto processType = XRE_GetProcessType();
-  for (uint32_t histogramId = 0; histogramId < HistogramCount; ++histogramId) {
-    const HistogramInfo& info = gHistogramInfos[histogramId];
-    if (info.keyed ||
-      info.histogramType != nsITelemetry::HISTOGRAM_FLAG ||
-      !CanRecordInProcess(info.record_in_processes, processType)) {
-      continue;
-    }
-
-    for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
-      if ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess) {
-        continue;
-      }
-
-      mozilla::DebugOnly<Histogram*> h = nullptr;
-      h = internal_GetHistogramById(HistogramID(histogramId), ProcessID(process), sessionType);
-      MOZ_ASSERT(h);
-    }
-  }
-
   for (uint32_t process = 0; process < static_cast<uint32_t>(ProcessID::Count); ++process) {
     JS::Rooted<JSObject*> processObject(cx, JS_NewPlainObject(cx));
     if (!processObject) {
       return NS_ERROR_FAILURE;
     }
     if (!JS_DefineProperty(cx, root_obj, GetNameForProcessID(ProcessID(process)),
                            processObject, JSPROP_ENUMERATE)) {
       return NS_ERROR_FAILURE;
@@ -1962,17 +1935,21 @@ TelemetryHistogram::CreateHistogramSnaps
 
       HistogramID id = HistogramID(i);
 
       if (!CanRecordInProcess(info.record_in_processes, ProcessID(process)) ||
         ((ProcessID(process) == ProcessID::Gpu) && !includeGPUProcess)) {
         continue;
       }
 
-      Histogram* h = internal_GetHistogramById(id, ProcessID(process), sessionType, /* instantiate = */ false);
+      bool shouldInstantiate =
+        info.histogramType == nsITelemetry::HISTOGRAM_FLAG;
+      Histogram* h = internal_GetHistogramById(id, ProcessID(process),
+                                               sessionType,
+                                               shouldInstantiate);
       if (!h || internal_IsExpired(h) || !internal_ShouldReflectHistogram(h, id)) {
         continue;
       }
 
       JS::Rooted<JSObject*> hobj(cx, JS_NewPlainObject(cx));
       if (!hobj) {
         return NS_ERROR_FAILURE;
       }
--- a/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetrySession.js
@@ -767,40 +767,40 @@ add_task(async function test_checkSubses
   ]);
 
   // Compare the two sets of histograms.
   // The "subsession" histograms should match the registered
   // "classic" histograms. However, histograms can change
   // between us collecting the different payloads, so we only
   // check for deep equality on known stable histograms.
   let checkHistograms = (classic, subsession, message) => {
-    for (let id of Object.keys(classic)) {
+    for (let id of Object.keys(subsession)) {
       if (!registeredIds.has(id)) {
         continue;
       }
 
-      Assert.ok(id in subsession, message + ` (${id})`);
+      Assert.ok(id in classic, message + ` (${id})`);
       if (stableHistograms.has(id)) {
         Assert.deepEqual(classic[id],
                          subsession[id], message);
       } else {
         Assert.equal(classic[id].histogram_type,
                      subsession[id].histogram_type, message);
       }
     }
   };
 
   // Same as above, except for keyed histograms.
   let checkKeyedHistograms = (classic, subsession, message) => {
-    for (let id of Object.keys(classic)) {
+    for (let id of Object.keys(subsession)) {
       if (!registeredIds.has(id)) {
         continue;
       }
 
-      Assert.ok(id in subsession, message);
+      Assert.ok(id in classic, message);
       if (stableKeyedHistograms.has(id)) {
         Assert.deepEqual(classic[id],
                          subsession[id], message);
       }
     }
   };
 
   // Both classic and subsession payload histograms should start the same.
@@ -883,19 +883,18 @@ add_task(async function test_checkSubses
   checkKeyedHistograms(classic.keyedHistograms, subsession.keyedHistograms, "Should be able to reset subsession keyed");
 
   // ... then check that the next snapshot shows the subsession
   // histograms got reset.
   classic = TelemetrySession.getPayload();
   subsession = TelemetrySession.getPayload("environment-change");
 
   Assert.ok(COUNT_ID in classic.histograms);
-  Assert.ok(COUNT_ID in subsession.histograms);
+  Assert.ok(!(COUNT_ID in subsession.histograms));
   Assert.equal(classic.histograms[COUNT_ID].sum, 1);
-  Assert.equal(subsession.histograms[COUNT_ID].sum, 0);
 
   Assert.ok(KEYED_ID in classic.keyedHistograms);
   Assert.ok(!(KEYED_ID in subsession.keyedHistograms));
   Assert.equal(classic.keyedHistograms[KEYED_ID]["a"].sum, 1);
   Assert.equal(classic.keyedHistograms[KEYED_ID]["b"].sum, 1);
 
   // Adding values should get picked up in both again.
   count.add(1);
@@ -1033,17 +1032,17 @@ add_task(async function test_dailyCollec
   ping = await PingServer.promiseNextPing();
   Assert.ok(!!ping);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
   Assert.equal(ping.payload.info.reason, REASON_DAILY);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), expectedDate.toISOString());
 
-  Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
+  Assert.ok(!(COUNT_ID in ping.payload.histograms));
   Assert.ok(!(KEYED_ID in ping.payload.keyedHistograms));
 
   // Trigger and collect another daily ping, with the histograms being set again.
   count.add(1);
   keyed.add("a", 1);
   keyed.add("b", 1);
 
   // The daily ping is rescheduled for "tomorrow".
@@ -1237,17 +1236,17 @@ add_task(async function test_environment
   Assert.ok(!!ping);
 
   Assert.equal(ping.type, PING_TYPE_MAIN);
   Assert.equal(ping.environment.settings.userPrefs[PREF_TEST], 1);
   Assert.equal(ping.payload.info.reason, REASON_ENVIRONMENT_CHANGE);
   subsessionStartDate = new Date(ping.payload.info.subsessionStartDate);
   Assert.equal(subsessionStartDate.toISOString(), startHour.toISOString());
 
-  Assert.equal(ping.payload.histograms[COUNT_ID].sum, 0);
+  Assert.ok(!(COUNT_ID in ping.payload.histograms));
   Assert.ok(!(KEYED_ID in ping.payload.keyedHistograms));
 
   await TelemetryController.testShutdown();
 });
 
 add_task(async function test_experimentAnnotations_subsession() {
   if (gIsAndroid) {
     // We don't split subsessions on environment changes yet on Android.