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
--- 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);
@@ -1045,17 +1044,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".
@@ -1249,17 +1248,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.