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

RESOLVED FIXED in Firefox 59

Status

()

Toolkit
Telemetry
P4
normal
RESOLVED FIXED
9 days ago
4 days ago

People

(Reporter: adbugger, Assigned: adbugger, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

9 days ago
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.
(Assignee)

Updated

9 days ago
Has STR: --- → irrelevant
Component: Themes → Telemetry
See Also: → bug 1364043
(Assignee)

Updated

9 days ago
Blocks: 1428893
(Assignee)

Updated

9 days ago
See Also: → bug 1428893
Mentor: chutten@mozilla.com
Priority: -- → P4
Whiteboard: [lang=c++]
(Assignee)

Comment 1

8 days ago
If it's alright, I'd like to work on this too.
Comment hidden (obsolete)
Comment hidden (typo)
(Assignee)

Updated

8 days ago
Attachment #8941177 - Attachment is obsolete: true
Attachment #8941177 - Flags: review?(chutten)
(Assignee)

Comment 4

8 days ago
Created attachment 8941219 [details] [diff] [review]
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.
Attachment #8941219 - Flags: review?(chutten)

Comment 5

7 days ago
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`
Attachment #8941219 - Flags: review?(chutten)

Comment 6

6 days ago
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.
(Assignee)

Comment 7

6 days ago
Thank you for clarifying that! I'll incorporate that into a test.
(Assignee)

Comment 8

6 days ago
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.
Flags: needinfo?(chutten)

Comment 9

6 days ago
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?
Flags: needinfo?(chutten)
(Assignee)

Comment 10

6 days ago
Created attachment 8941908 [details] [diff] [review]
Added test cases for keyed histograms
Attachment #8941219 - Attachment is obsolete: true
Attachment #8941908 - Flags: review?(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?
Attachment #8941908 - Flags: review?(chutten)
(Assignee)

Comment 12

6 days ago
Created attachment 8941946 [details] [diff] [review]
Allow C++ to add multiple samples to a keyed histogram in one call.
Attachment #8941908 - Attachment is obsolete: true
Attachment #8941946 - Flags: review?(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!
Attachment #8941946 - Flags: review?(chutten) → review+
Marking for checkin.

Do you need help finding your next bug? I'm pretty sure you know where to look, but just in case...
Keywords: checkin-needed
(Assignee)

Comment 15

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

Comment 17

5 days ago
Created attachment 8941978 [details] [diff] [review]
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.
Attachment #8941946 - Attachment is obsolete: true
Attachment #8941978 - Flags: review?(chutten)
(Assignee)

Comment 18

5 days ago
Builds clean on try.
(Assignee)

Updated

5 days ago
Assignee: nobody → adibhar97

Updated

5 days ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Updated

5 days ago
Attachment #8941978 - Flags: review?(chutten) → review+
(Assignee)

Updated

5 days ago
Keywords: checkin-needed

Comment 19

5 days ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5590c751f6d8
Allow C++ to accumulate multiple samples into a keyed histogram in one call r=chutten
Keywords: checkin-needed

Comment 20

4 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5590c751f6d8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.