Add C++ test coverage for the Categorical Histogram

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Dexter, Assigned: david, Mentored)

Tracking

Trunk
mozilla58
Points:
2

Firefox Tracking Flags

(firefox55 affected, firefox58 fixed)

Details

(Whiteboard: [measurement:client][lang=c++])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

2 years ago
We should add test coverage for the keyed version of the AccumulateCategorical/Accumulate functions [1]. We can use the "TELEMETRY_TEST_CATEGORICAL" for testing.

The new test should be added to the TestHistograms.cpp file in toolkit/components/telemetry/tests/gtest and should:

- Get a reference to the histogram.
- Clear its content.
- Store a categorical value
- Get a snapshot of the histogram.
- Check that the histogram contains the stored values.

The code can be tested by using the following command:

> ./mach gtest Telemetry*

[1] - http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/toolkit/components/telemetry/Telemetry.h#108,124
(Reporter)

Comment 1

2 years ago
Federico, would you be interested in taking on this one?
Points: --- → 2
Depends on: 1312698
Flags: needinfo?(federico_padua)
Priority: -- → P3
Whiteboard: [measurement:client][lang=c++]
(In reply to Alessio Placitelli [:Dexter] from comment #1)
> Federico, would you be interested in taking on this one?

I can take it and try, but I will only have time during the weekend for that this week...but I'll be happy to do it!
Flags: needinfo?(federico_padua)
(Reporter)

Comment 3

2 years ago
(In reply to Federico Padua [:fedepad] from comment #2)
> (In reply to Alessio Placitelli [:Dexter] from comment #1)
> > Federico, would you be interested in taking on this one?
> 
> I can take it and try, but I will only have time during the weekend for that
> this week...but I'll be happy to do it!

No rush, take your time! Feel free to refactor the TestHistograms.cpp file as you see fit!

http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
Assignee: nobody → federico_padua
(Reporter)

Comment 4

2 years ago
Federico, are you still interested in working on this bug?
Flags: needinfo?(federico_padua)
(Reporter)

Updated

2 years ago
Assignee: federico_padua → nobody
Flags: needinfo?(federico_padua)
Bug 1357682 also adds an overload for AccumulateCategorical() for keyed histograms.
We should also cover that.
(Assignee)

Comment 6

2 years ago
Alessio: mind if I work on this one?
(Reporter)

Comment 7

2 years ago
(In reply to David Krauser from comment #6)
> Alessio: mind if I work on this one?

Not at all! I've assigned the bug to you, thanks for your interest (sorry for the delay, I was out last week).

You can find some information about how to work on this bug in comment 0, let me know if there's any doubt or anything else we should discuss!
Assignee: nobody → david
Flags: needinfo?(david)
(Assignee)

Comment 8

2 years ago
Awesome. I'll let you know if I hit any issues :-)
Flags: needinfo?(david)
(Reporter)

Comment 9

2 years ago
Hi David, how is this progressing? Are you still working on it? Do you need any help?
Flags: needinfo?(david)
(Assignee)

Comment 10

2 years ago
Alessio: Yes, sorry! Don't give up hope, yet - I should have a first pass this week.
Flags: needinfo?(david)
(Assignee)

Comment 11

2 years ago
Posted patch 1351274.patch (obsolete) — Splinter Review
Sorry this took so long. Is this the right approach here?
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 12

2 years ago
Comment on attachment 8909065 [details] [diff] [review]
1351274.patch

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

That's a very good start, thanks David! I now see how the C++ categorical API could use some more documentation :-) The tests look clear and clean, however they are not testing the API properly. With these changes, we should get on the right track ;) Thanks for the work so far! Please feel free to reach out if you have any other question!

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +173,5 @@
>  }
> +
> +TEST_F(TelemetryTestFixture, AccumulateCategoricalHistogram)
> +{
> +  const Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL kExpectedValue =

This is not an expected value but an identifier for the categorical histogram we want to modify. I can see how this is confusing though, the header file don't provide much info! See http://searchfox.org/mozilla-central/rev/05c4c3bc0cfb9b0fc66bdfc8c47cac674e45f151/toolkit/components/telemetry/Telemetry.h#101,117,134

@@ +182,5 @@
> +  GetAndClearHistogram(cx.GetJSContext(), mTelemetry,
> +                       NS_LITERAL_CSTRING("TELEMETRY_TEST_CATEGORICAL"), false);
> +
> +  // Accumulate in the histogram
> +  Telemetry::AccumulateCategorical(kExpectedValue);

This is basically saying "Accumulate one unit into the categorical histogram with label Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel". The value to accumulate is always 1 as far as I can tell for the c++ categorical histograms. I would add a comment above this line to explain that and then explicitly call the accumulation function like that:

Telemetry::AccumulateCategorical(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel);

Since the categorical histogram c++ API allows for two different ways of accumulating, I would additionally try to accumulate in the same label using the string version of the API:

Telemetry::AccumulateCategorical(Telemetry::TELEMETRY_TEST_CATEGORICAL, NS_LITERAL_CSTRING("CommonLabel"));

At the end of this test, we would check if that label for the categorical histogram contains the value of 2 (you can define kExpectedValue as 2).

@@ +204,5 @@
> +}
> +
> +TEST_F(TelemetryTestFixture, AccumulateKeyedCategoricalHistogram)
> +{
> +  const Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL kExpectedValue =

The same comment for the other test applies here: this is the cateogrical label, not the expected value. Just pass this to the accumulate function below.

@@ +234,5 @@
> +
> +  // Check that the "sum" stored in the histogram matches with |kExpectedValue|
> +  uint32_t uSum = 0;
> +  JS::ToUint32(cx.GetJSContext(), sum, &uSum);
> +  ASSERT_EQ(uSum, static_cast<uint32_t>(kExpectedValue)) << "The histogram is not returning expected value";

The expected value in this case should be 1, since you're only performing a single accumulation on that key/label pair.
Attachment #8909065 - Flags: feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 13

2 years ago
That makes sense. I got lucky that my tests passed - I just happened to choose an enum with the value of 1. I'll make some changes.
(Assignee)

Comment 14

2 years ago
Posted patch 1351274-2.patch (obsolete) — Splinter Review
How's this look?
Attachment #8909065 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
(Reporter)

Comment 15

2 years ago
Comment on attachment 8912930 [details] [diff] [review]
1351274-2.patch

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

Great job, this looks on the right track now! There are a few smaller issues left, let's iron them out.

One note: when requesting a review of a patch, instead of adding a need-info for me, you can also set me as a reviewer in the form you use to attach it.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +42,5 @@
>  }
>  
>  void
> +GetElement(JSContext* cx, uint32_t index, JS::HandleValue valueIn,
> +                             JS::MutableHandleValue valueOut)

nit: please align this with |JSContext| - I know this is identical to |GetProperty|, but the other function is wrong :P Since you're here, would you mind fixing indentation there as well?

@@ +196,5 @@
> +  // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel
> +  Telemetry::AccumulateCategorical(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel);
> +
> +  // Accumulate another unit into the categorical histogram with label
> +  // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel using the string API 

nit: rephrase this as "Accumulate another unit into the same categorical histogram using a string label". Let's also remove the trailing whitespace

@@ +208,5 @@
> +  // Get our histogram from the snapshot
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  GetProperty(cx.GetJSContext(), "TELEMETRY_TEST_CATEGORICAL", snapshot, &histogram);
> +
> +  // Get counts array from histogram. Each entry in the array maps to a label in the histogram. 

nit: remove the trailing whitespace

@@ +234,5 @@
> +  GetAndClearHistogram(cx.GetJSContext(), mTelemetry,
> +                       NS_LITERAL_CSTRING("TELEMETRY_TEST_KEYED_CATEGORICAL"), true);
> +
> +  // Accumulate one unit into the categorical histogram with label
> +  // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel

nit: that's LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL (it's missing KEYED)

@@ +236,5 @@
> +
> +  // Accumulate one unit into the categorical histogram with label
> +  // Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel
> +  Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"),
> +                                        Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel);

Let's accumulate at least in 2 keys (with different values) and make sure to get the correct values out.

@@ +250,5 @@
> +  // Get "sample" property from histogram
> +  JS::RootedValue sample(cx.GetJSContext());
> +  GetProperty(cx.GetJSContext(), "sample", histogram,  &sample);
> +
> +  // Get counts array from the sample. Each entry in the array maps to a label in the histogram. 

nit: remove the trailing whitespace

@@ +257,5 @@
> +
> +  // Get the value for the label we care about
> +  JS::RootedValue value(cx.GetJSContext());
> +  GetElement(cx.GetJSContext(),
> +             static_cast<uint32_t>(Telemetry::LABELS_TELEMETRY_TEST_CATEGORICAL::CommonLabel),

The label here should be Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel - the labels happen to have the same index.
Attachment #8912930 - Flags: feedback+
(Reporter)

Updated

2 years ago
Flags: needinfo?(alessio.placitelli)
(Assignee)

Comment 16

2 years ago
Posted patch 1351274-3.patch (obsolete) — Splinter Review
Attachment #8912930 - Attachment is obsolete: true
Attachment #8914111 - Flags: review+
(Assignee)

Updated

2 years ago
Attachment #8914111 - Flags: review+
(Assignee)

Comment 17

2 years ago
Posted patch 1351274-4.patch (obsolete) — Splinter Review
Attachment #8914111 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8914112 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 18

2 years ago
Comment on attachment 8914112 [details] [diff] [review]
1351274-4.patch

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

This looks good, we only need to make one little change to the second test and we're done!

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +238,5 @@
> +  Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"),
> +                                        Telemetry::LABELS_TELEMETRY_TEST_KEYED_CATEGORICAL::CommonLabel);
> +
> +  // Accumulate another unit into the same categorical histogram
> +  Telemetry::AccumulateCategoricalKeyed(NS_LITERAL_CSTRING("sample"),

We should use a different key here, as we want to make sure that the keying system works with categoricals as well. Let's just swap "sample" with "other-key" and change the rest of the test to check that both keys have the right value.
Attachment #8914112 - Flags: review?(alessio.placitelli) → feedback+
(Assignee)

Comment 19

2 years ago
Attachment #8914112 - Attachment is obsolete: true
Attachment #8914578 - Flags: review?(alessio.placitelli)
(Reporter)

Comment 20

2 years ago
Comment on attachment 8914578 [details] [diff] [review]
1351274-5.patch

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

This looks good now, thanks for your efforts David!
Attachment #8914578 - Flags: review?(alessio.placitelli) → review+

Comment 23

2 years ago
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba149aead553
Add C++ test coverage for the Categorical Histogram. r=dexter
(Reporter)

Comment 24

2 years ago
David, would you be interested in taking on another bug? There are a few options depending on the language of choice. For example, for C++ one valuable bug for us would be bug 1393041.
Flags: needinfo?(david)

Comment 25

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ba149aead553
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
(Reporter)

Updated

a year ago
Flags: needinfo?(david)
You need to log in before you can comment on or make changes to this bug.