bug 1366294 - Part 10 - Nail down count histogram semantics. r=gfritzsche
☠☠ backed out by 9f869d8dc3e3 ☠ ☠
authorChris H-C <chutten@mozilla.com>
Tue, 04 Jul 2017 10:12:23 -0400
changeset 418919 09043ee913d4d4c3ccee9187834aa33c54905393
parent 418918 0ed44ecf9062ac9486cd379870e2376f3a8b119a
child 418920 140948f5b9555f9ff94503e671730e3afaede284
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgfritzsche
bugs1366294
milestone56.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 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
 
@@ -950,27 +950,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
 
 
 ////////////////////////////////////////////////////////////////////////
@@ -1917,38 +1912,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;
@@ -1961,17 +1934,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
@@ -775,40 +775,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.
@@ -891,19 +891,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);
@@ -1052,17 +1051,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".
@@ -1256,17 +1255,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.