Bug 1468809 - Do not snapshot expired keyed histograms. r?chutten,janerik
This patch changes the snapshotting code for keyed histograms
so that requesting a snapshot does not report expired histograms.
MozReview-Commit-ID: GDiw6yOcF8J
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -7414,16 +7414,28 @@
},
"TELEMETRY_TEST_EXPIRED": {
"record_in_processes": ["main", "content"],
"alert_emails": ["telemetry-client-dev@mozilla.com"],
"expires_in_version": "4",
"kind": "flag",
"description": "a testing histogram; not meant to be touched"
},
+ "TELEMETRY_TEST_EXPIRED_KEYED": {
+ "record_in_processes": ["main", "content"],
+ "alert_emails": ["telemetry-client-dev@mozilla.com"],
+ "expires_in_version": "4",
+ "kind": "linear",
+ "keyed": true,
+ "low": 1,
+ "high": 2147483646,
+ "n_buckets": 10,
+ "bug_numbers": [1468809],
+ "description": "a testing histogram; not meant to be touched"
+ },
"TELEMETRY_TEST_CONTENT_PROCESS": {
"record_in_processes": ["content"],
"alert_emails": ["telemetry-client-dev@mozilla.com"],
"expires_in_version": "never",
"kind": "linear",
"low": 1,
"high": 10000,
"n_buckets": 10,
--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
@@ -174,17 +174,17 @@ struct KeyedHistogramSnapshotInfo {
HistogramID histogramId;
};
typedef mozilla::Vector<KeyedHistogramSnapshotInfo> KeyedHistogramSnapshotsArray;
typedef mozilla::Vector<KeyedHistogramSnapshotsArray> KeyedHistogramProcessSnapshotsArray;
class KeyedHistogram {
public:
- KeyedHistogram(HistogramID id, const HistogramInfo& info);
+ KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired);
~KeyedHistogram();
nsresult GetHistogram(const nsCString& name, Histogram** histogram);
Histogram* GetHistogram(const nsCString& name);
uint32_t GetHistogramType() const { return mHistogramInfo.histogramType; }
nsresult GetJSKeys(JSContext* cx, JS::CallArgs& args);
// Note: unlike other methods, GetJSSnapshot is thread safe.
nsresult GetJSSnapshot(JSContext* cx, JS::Handle<JSObject*> obj,
bool clearSubsession);
@@ -193,23 +193,26 @@ public:
nsresult Add(const nsCString& key, uint32_t aSample, ProcessID aProcessType);
void Clear();
HistogramID GetHistogramID() const { return mId; }
bool IsEmpty() const { return mHistogramMap.IsEmpty(); }
+ bool IsExpired() const { return mIsExpired; }
+
private:
typedef nsBaseHashtableET<nsCStringHashKey, Histogram*> KeyedHistogramEntry;
typedef AutoHashtable<KeyedHistogramEntry> KeyedHistogramMapType;
KeyedHistogramMapType mHistogramMap;
const HistogramID mId;
const HistogramInfo& mHistogramInfo;
+ bool mIsExpired;
};
} // namespace
////////////////////////////////////////////////////////////////////////
////////////////////////////////////////////////////////////////////////
//
@@ -232,16 +235,19 @@ Histogram** gHistogramStorage;
KeyedHistogram** gKeyedHistogramStorage;
// Cache of histogram name to a histogram id.
StringToHistogramIdMap gNameToHistogramIDMap(HistogramCount);
// To simplify logic below we use a single histogram instance for all expired histograms.
Histogram* gExpiredHistogram = nullptr;
+// The single placeholder for expired keyed histograms.
+KeyedHistogram* gExpiredKeyedHistogram = nullptr;
+
// This tracks whether recording is enabled for specific histograms.
// To utilize C++ initialization rules, we invert the meaning to "disabled".
bool gHistogramRecordingDisabled[HistogramCount] = {};
// This is for gHistogramInfos, gHistogramStringTable
#include "TelemetryHistogramData.inc"
} // namespace
@@ -379,17 +385,31 @@ internal_GetKeyedHistogramById(Histogram
KeyedHistogram* kh = internal_GetKeyedHistogramFromStorage(histogramId,
processId);
if (kh || !instantiate) {
return kh;
}
const HistogramInfo& info = gHistogramInfos[histogramId];
- kh = new KeyedHistogram(histogramId, info);
+ const bool isExpired = IsExpiredVersion(info.expiration());
+
+ // If the keyed histogram is expired, set its storage to the expired
+ // keyed histogram.
+ if (isExpired) {
+ if (!gExpiredKeyedHistogram) {
+ // If we don't have an expired keyed histogram, create one.
+ gExpiredKeyedHistogram = new KeyedHistogram(histogramId, info, true /* expired */);
+ MOZ_ASSERT(gExpiredKeyedHistogram);
+ }
+ kh = gExpiredKeyedHistogram;
+ } else {
+ kh = new KeyedHistogram(histogramId, info, false /* expired */);
+ }
+
internal_SetKeyedHistogramInStorage(histogramId, processId, kh);
return kh;
}
// Look up a histogram id from a histogram name.
nsresult
internal_GetHistogramIdByName(const StaticMutexAutoLock& aLock,
@@ -894,20 +914,21 @@ internal_ReflectKeyedHistogram(const Key
histogramSnapshot, JSPROP_ENUMERATE)) {
return NS_ERROR_FAILURE;
}
}
return NS_OK;
}
-KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info)
+KeyedHistogram::KeyedHistogram(HistogramID id, const HistogramInfo& info, bool expired)
: mHistogramMap()
, mId(id)
, mHistogramInfo(info)
+ , mIsExpired(expired)
{
}
KeyedHistogram::~KeyedHistogram()
{
for (auto iter = mHistogramMap.Iter(); !iter.Done(); iter.Next()) {
Histogram* h = iter.Get()->mData;
if (h == gExpiredHistogram) {
@@ -964,16 +985,21 @@ KeyedHistogram::Add(const nsCString& key
internal_CanRecordExtended());
// If `histogram` is a non-parent-process histogram, then recording-enabled
// has been checked in its owner process.
if (!canRecordDataset ||
(aProcessType == ProcessID::Parent && !internal_IsRecordingEnabled(mId))) {
return NS_OK;
}
+ // Don't record if expired.
+ if (IsExpired()) {
+ return NS_OK;
+ }
+
// Don't record if the current platform is not enabled
if (!CanRecordProduct(gHistogramInfos[mId].products)) {
return NS_OK;
}
Histogram* histogram = GetHistogram(key);
MOZ_ASSERT(histogram);
if (!histogram) {
@@ -1129,17 +1155,17 @@ internal_GetKeyedHistogramsSnapshot(cons
if (!IsInDataset(info.dataset, aDataset)) {
continue;
}
KeyedHistogram* keyed = internal_GetKeyedHistogramById(id,
ProcessID(process),
/* instantiate = */ false);
- if (!keyed || (aSkipEmpty && keyed->IsEmpty())) {
+ if (!keyed || (aSkipEmpty && keyed->IsEmpty()) || keyed->IsExpired()) {
continue;
}
// Take a snapshot of the keyed histogram data!
KeyedHistogramSnapshotData snapshot;
if (!NS_SUCCEEDED(keyed->GetSnapshot(aLock, snapshot, aClearSubsession))) {
return NS_ERROR_FAILURE;
}
@@ -1995,28 +2021,31 @@ void TelemetryHistogram::DeInitializeGlo
gCanRecordBase = false;
gCanRecordExtended = false;
gNameToHistogramIDMap.Clear();
gInitDone = false;
// FactoryGet `new`s Histograms for us, but requires us to manually delete.
if (XRE_IsParentProcess()) {
for (size_t i = 0; i < HistogramCount * size_t(ProcessID::Count); ++i) {
- if (i < HistogramCount * size_t(ProcessID::Count)) {
+ if (i < HistogramCount * size_t(ProcessID::Count)
+ && gKeyedHistogramStorage[i] != gExpiredKeyedHistogram) {
delete gKeyedHistogramStorage[i];
}
if (gHistogramStorage[i] != gExpiredHistogram) {
delete gHistogramStorage[i];
}
}
delete[] gHistogramStorage;
delete[] gKeyedHistogramStorage;
}
delete gExpiredHistogram;
gExpiredHistogram = nullptr;
+ delete gExpiredKeyedHistogram;
+ gExpiredKeyedHistogram = nullptr;
}
#ifdef DEBUG
bool TelemetryHistogram::GlobalStateHasBeenInitialized() {
StaticMutexAutoLock locker(gTelemetryHistogramMutex);
return gInitDone;
}
#endif
--- a/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
+++ b/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ -450,16 +450,33 @@ add_task(async function test_expired_his
}
Assert.equal(histograms[process].__expired__, undefined);
}
let parentHgrams = Telemetry.snapshotHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
false /* clear */).parent;
Assert.equal(parentHgrams[test_expired_id], undefined);
});
+add_task(async function test_keyed_expired_histogram() {
+ var test_expired_id = "TELEMETRY_TEST_EXPIRED_KEYED";
+ var dummy = Telemetry.getKeyedHistogramById(test_expired_id);
+ dummy.add("someKey", 1);
+
+ const histograms = Telemetry.snapshotKeyedHistograms(Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN,
+ false /* clear */);
+ for (let process of ["parent", "content", "gpu", "extension"]) {
+ if (!(process in histograms)) {
+ info("Nothing present for process " + process);
+ continue;
+ }
+ Assert.ok(!(test_expired_id in histograms[process]),
+ "The expired keyed histogram must not be reported");
+ }
+});
+
add_task(async function test_keyed_histogram() {
// Check that invalid names get rejected.
let threw = false;
try {
Telemetry.getKeyedHistogramById("test::unknown histogram", "never", Telemetry.HISTOGRAM_BOOLEAN);
} catch (e) {
// This should throw as it is an unknown ID