1428885 - Allow C++ to accumulate multiple samples into a keyed histogram in one call

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P4)

Description

[Aditya Bharti [:adbugger]]

As per bug 1364043, the C++ Telemetry::Accumulate function was extended to support adding multiple samples to a histogram in one call. This bug is to extend the same support for keyed histograms. See comment #27 on bug 1364043 for more information.

Comment 1

[Aditya Bharti [:adbugger]]

If it's alright, I'd like to work on this too.

Comment 2

[Aditya Bharti [:adbugger]]

Attachment: Telemetry::Accumulate accumulates multiple samples into a keyed histogram

Extension of the Telemetry::Accumulate API based on the patch for bug 1364043. If everything looks ok, I'll write the tests next.

Ignore the 'TODO's for now. I'll remove them in the final patch.

Comment 3

[Aditya Bharti [:adbugger]]

Comment on attachment 8941177 [details] [diff] [review]
Telemetry::Accumulate accumulates multiple samples into a keyed histogram

>diff --git a/toolkit/components/telemetry/Telemetry.cpp b/toolkit/components/telemetry/Telemetry.cpp
>--- a/toolkit/components/telemetry/Telemetry.cpp
>+++ b/toolkit/components/telemetry/Telemetry.cpp
>@@ -1942,6 +1942,12 @@ Accumulate(HistogramID aID, const nsCStr
>   TelemetryHistogram::Accumulate(aID, aKey, aSample);
> }
> 
>+void // TODO: bug 1428885
>+Accumulate(HistogramID aID, const nsCString& aKey, const nsTArray<uint32_t>& aSamples)
>+{
>+  TelemetryHistogram::Accumulate(aID, aKey, aSamples);
>+}
>+
> void
> Accumulate(const char* name, uint32_t sample)
> {
>diff --git a/toolkit/components/telemetry/Telemetry.h b/toolkit/components/telemetry/Telemetry.h
>--- a/toolkit/components/telemetry/Telemetry.h
>+++ b/toolkit/components/telemetry/Telemetry.h
>@@ -75,6 +75,14 @@ void Accumulate(HistogramID id, const ns
> void Accumulate(HistogramID id, const nsCString& key, uint32_t sample = 1);
> 
> /**
>+ * Adds an array of samples to a histogram defined in TelemetryHistograms.h
>+ * @param id - histogram id
>+ * @param samples - values to record.
>+ * TODO: bug 1428885
>+ */
>+void Accumulate(HistogramID id, const nsCString& key, const nsTArray<uint32_t> samples);
>+
>+/**
>  * Adds a sample to a histogram defined in TelemetryHistogramEnums.h.
>  * This function is here to support telemetry measurements from Java,
>  * where we have only names and not numeric IDs.  You should almost
>diff --git a/toolkit/components/telemetry/TelemetryHistogram.cpp b/toolkit/components/telemetry/TelemetryHistogram.cpp
>--- a/toolkit/components/telemetry/TelemetryHistogram.cpp
>+++ b/toolkit/components/telemetry/TelemetryHistogram.cpp
>@@ -1915,6 +1915,36 @@ TelemetryHistogram::Accumulate(Histogram
>   internal_Accumulate(aID, aKey, aSample);
> }
> 
>+void // TODO: bug 1428885
>+TelemetryHistogram::Accumulate(HistogramID aID, const nsCString& aKey,
>+                                    const nsTArray<uint32_t> aSamples)
>+{
>+  if (NS_WARN_IF(!internal_IsHistogramEnumId(aID))) {
>+    MOZ_ASSERT_UNREACHABLE("Histogram usage requires valid ids");
>+    return;
>+  }
>+
>+  // Check that this histogram is keyed
>+  MOZ_ASSERT(gHistogramInfos[aID].keyed, "Cannot accumulate into a non-keyed histogram using a key.");
>+
>+  // Check if we're allowed to record in the provided key, for this histogram.
>+  if (!gHistogramInfos[aID].allows_key(aKey)) {
>+    nsPrintfCString msg("%s - key '%s' not allowed for this keyed histogram",
>+                        gHistogramInfos[aID].name(),
>+                        aKey.get());
>+    LogToBrowserConsole(nsIScriptError::errorFlag, NS_ConvertUTF8toUTF16(msg));
>+    TelemetryScalar::Add(
>+      mozilla::Telemetry::ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS,
>+      NS_ConvertASCIItoUTF16(gHistogramInfos[aID].name()), 1);
>+    return;
>+  }
>+
>+  StaticMutexAutoLock locker(gTelemetryHistogramMutex);
>+  for(uint32_t sample: aSamples){
>+    internal_Accumulate(aID, aKey, sample);
>+  }
>+}
>+
> void
> TelemetryHistogram::Accumulate(const char* name, uint32_t sample)
> {
>diff --git a/toolkit/components/telemetry/TelemetryHistogram.h b/toolkit/components/telemetry/TelemetryHistogram.h
>--- a/toolkit/components/telemetry/TelemetryHistogram.h
>+++ b/toolkit/components/telemetry/TelemetryHistogram.h
>@@ -39,6 +39,8 @@ void Accumulate(mozilla::Telemetry::Hist
> void Accumulate(mozilla::Telemetry::HistogramID aHistogram, const nsTArray<uint32_t>& aSamples);
> void Accumulate(mozilla::Telemetry::HistogramID aID, const nsCString& aKey,
>                                             uint32_t aSample);
>+void Accumulate(mozilla::Telemetry::HistogramID aID, const nsCString& aKey,
>+                                          const nsTArray<uint32_t>& aSamples); // TODO: bug 1428885
> void Accumulate(const char* name, uint32_t sample);
> void Accumulate(const char* name, const nsCString& key, uint32_t sample);
>

Comment 4

[Aditya Bharti [:adbugger]]

Attachment: Telemetry::Accumulate accumulates multiple samples into a keyed histogram

Previous patch had a typo.

Extended the Telemetry::Accumulate function to accumulate multiple samples to keyed histograms. Patch based on bug 1364043. Builds successfully.

Will remove TODOs in final patch.

Comment 5

[Chris H-C :chutten]

Comment on attachment 8941219 [details] [diff] [review]
Telemetry::Accumulate accumulates multiple samples into a keyed histogram

Review of attachment 8941219 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good so far.

::: toolkit/components/telemetry/Telemetry.h
@@ +76,5 @@
>  
>  /**
> + * Adds an array of samples to a histogram defined in TelemetryHistograms.h
> + * @param id - histogram id
> + * @param samples - values to record.

Also need a @param for `key`

Comment 6

[Chris H-C :chutten]

On IRC you asked "For attempts to accumulate multiple samples at a time [to a key that isn't allowed], should I accumulate the length of the sample array, or just the default 1 [into ScalarID::TELEMETRY_ACCUMULATE_UNKNOWN_HISTOGRAM_KEYS]?"

I bothered :Dexter about it just now and he says "the original idea was to count the number of places in which that was happening"

So accumulating the default 1 is fulfills the original plan for the scalar.

Comment 7

[Aditya Bharti [:adbugger]]

Thank you for clarifying that! I'll incorporate that into a test.

Comment 8

[Aditya Bharti [:adbugger]]

It appears that the only histogram with a set of allowed keys is TELEMETRY_TEST_KEYED_KEYS, which is a boolean histogram and not covered by the extended API. The other TELEMETRY_TEST_KEYED_* histograms allow all keys to be accumulated.

So options are:
1. Leave this particular test case (counting the number of calls with an unknown key) uncovered.
2. Define another histogram.
3. Extend the API to support boolean values.

Leaving this test case for now. Will continue with other tests and pick this up once ni? is cleared.

Comment 9

[Chris H-C :chutten]

Telemetry::Accumulate does support boolean histograms. It just coerces them to uint32_t on the way through. This API will require that the caller create an nsTArray<uint32_t> for storing the samples, but they can append bool to the array and the values will coerce, no?

Does nsTArray<uint32_t> samples({true, false, false}); work, in other words?

Comment 10

[Aditya Bharti [:adbugger]]

Attachment: Added test cases for keyed histograms

Comment 11

[Chris H-C :chutten]

Comment on attachment 8941908 [details] [diff] [review]
Added test cases for keyed histograms

Review of attachment 8941908 [details] [diff] [review]:
-----------------------------------------------------------------

Just a couple of things.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +352,5 @@
> +
> +TEST_F(TelemetryTestFixture, AccumulateKeyedCountHistogram_MultipleSamples)
> +{
> +  const nsTArray<uint32_t> samples({5, 10, 15});
> +  const uint32_t kExpectedSum = 5+10+15;

Mozilla style prefers spaces between operators and arguments. So `5 + 10 + 15`

@@ +498,5 @@
> +    << "The histogram did not accumulate the correct number of 'true' booleans for key 'testkey'";
> +  ASSERT_EQ(uCountOther, 0)
> +    << "The histogram did not accumulate the correct number of undefined values for key 'testkey'";
> +
> +  // Get "CommonKey" property from histogram and check that it stores no data.

shouldn't this be the "not-allowed" key?

Comment 12

[Aditya Bharti [:adbugger]]

Attachment: Allow C++ to add multiple samples to a keyed histogram in one call.

Comment 13

[Chris H-C :chutten]

Comment on attachment 8941946 [details] [diff] [review]
Allow C++ to add multiple samples to a keyed histogram in one call.

Review of attachment 8941946 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!

Comment 14

[Chris H-C :chutten]

Marking for checkin.

Do you need help finding your next bug? I'm pretty sure you know where to look, but just in case...

Comment 15

[Aditya Bharti [:adbugger]]

I do have a general idea of where to look, but if you had something specific in mind, that's always appreciated. :)

Comment 16

[Chris H-C :chutten]

Failures on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65de93759c76f8a6b73f30c58421d84472bfd061

Comment 17

[Aditya Bharti [:adbugger]]

Attachment: Made changes. Should pass try now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fbed92f4defc83af7bb0ace28d3670ef30cb380

Should probably wait for try builds to pass this time. Meanwhile, I'll apply for editbugs permission.

Comment 18

[Aditya Bharti [:adbugger]]

Builds clean on try.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5590c751f6d8
Allow C++ to accumulate multiple samples into a keyed histogram in one call r=chutten

Comment 20

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/5590c751f6d8