Closed Bug 1351274 Opened 7 years ago Closed 7 years ago

Add C++ test coverage for the Categorical Histogram

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement
Points:
2

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox55 --- affected
firefox58 --- fixed

People

(Reporter: Dexter, Assigned: david, Mentored)

References

Details

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

Attachments

(1 file, 4 obsolete files)

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
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)
(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
Federico, are you still interested in working on this bug?
Flags: needinfo?(federico_padua)
Assignee: federico_padua → nobody
Flags: needinfo?(federico_padua)
Bug 1357682 also adds an overload for AccumulateCategorical() for keyed histograms.
We should also cover that.
Alessio: mind if I work on this one?
(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)
Awesome. I'll let you know if I hit any issues :-)
Flags: needinfo?(david)
Hi David, how is this progressing? Are you still working on it? Do you need any help?
Flags: needinfo?(david)
Alessio: Yes, sorry! Don't give up hope, yet - I should have a first pass this week.
Flags: needinfo?(david)
Attached patch 1351274.patch (obsolete) — Splinter Review
Sorry this took so long. Is this the right approach here?
Flags: needinfo?(alessio.placitelli)
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+
Flags: needinfo?(alessio.placitelli)
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.
Attached patch 1351274-2.patch (obsolete) — Splinter Review
How's this look?
Attachment #8909065 - Attachment is obsolete: true
Flags: needinfo?(alessio.placitelli)
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+
Flags: needinfo?(alessio.placitelli)
Attached patch 1351274-3.patch (obsolete) — Splinter Review
Attachment #8912930 - Attachment is obsolete: true
Attachment #8914111 - Flags: review+
Attachment #8914111 - Flags: review+
Attached patch 1351274-4.patch (obsolete) — Splinter Review
Attachment #8914111 - Attachment is obsolete: true
Attachment #8914112 - Flags: review?(alessio.placitelli)
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+
Attached patch 1351274-5.patchSplinter Review
Attachment #8914112 - Attachment is obsolete: true
Attachment #8914578 - Flags: review?(alessio.placitelli)
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+
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
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)
https://hg.mozilla.org/mozilla-central/rev/ba149aead553
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Flags: needinfo?(david)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: