1345861 - Add C++ test coverage for the Keyed Count Histogram

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Alessio Placitelli [:Dexter]]

Now that bug 1312698 landed and laid the foundations for the work, we can extend the histogram test coverage.

We should add test coverage for the keyed version of the Accumulate function [1]. We can use the "TELEMETRY_TEST_KEYED_COUNT" 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 different value in a few different keys.
- 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/source/toolkit/components/telemetry/Telemetry.h#75

Comment 1

[Alessio Placitelli [:Dexter]]

Would you be interested in taking this bug?

Comment 2

[Federico Padua [:fedepad]]

I have made some changes to TestHistograms.cpp for this and when I try to run the tests I get:

firefox-dev@firefox-dev:~/mozilla-central$ ./mach gtest Telemetry*
 0:00.25 /usr/bin/make -C testing/gtest -j2 -s gtest
 0:00.29 /home/firefox-dev/mozilla-central/objdir-frontend/dist/bin/firefox -unittest
Couldn't load XPCOM.

Do you know the reason?
Because when I just run the built firefox it seems ok...

Comment 3

[Avikalpa Kundu [:kalpa]]

I can work on it.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to Federico Padua (fedepad) from comment #2)
> I have made some changes to TestHistograms.cpp for this and when I try to
> run the tests I get:
> 
> firefox-dev@firefox-dev:~/mozilla-central$ ./mach gtest Telemetry*
>  0:00.25 /usr/bin/make -C testing/gtest -j2 -s gtest
>  0:00.29 /home/firefox-dev/mozilla-central/objdir-frontend/dist/bin/firefox
> -unittest
> Couldn't load XPCOM.
> 
> Do you know the reason?
> Because when I just run the built firefox it seems ok...

This is odd. Is that on artifact builds? Anyway, please leave this bug to Avikalpa for now, as he already have some domain knowledge for this. I will file a new bug tomorrow about adding a new test to this file and flag you on that one, if you're interested.

Comment 5

[Federico Padua [:fedepad]]

(In reply to Alessio Placitelli [:Dexter] from comment #4)
> (In reply to Federico Padua (fedepad) from comment #2)
> > I have made some changes to TestHistograms.cpp for this and when I try to
> > run the tests I get:
> > 
> > firefox-dev@firefox-dev:~/mozilla-central$ ./mach gtest Telemetry*
> >  0:00.25 /usr/bin/make -C testing/gtest -j2 -s gtest
> >  0:00.29 /home/firefox-dev/mozilla-central/objdir-frontend/dist/bin/firefox
> > -unittest
> > Couldn't load XPCOM.
> > 
> > Do you know the reason?
> > Because when I just run the built firefox it seems ok...
> 
> This is odd. Is that on artifact builds? 

I solved it. I had built a debug version with a special mozconfig. Setting that correctly made the testsuite run.
(MOZCONFIG=.mozconfig-debug ./mach gtest Telemetry*) 

> Anyway, please leave this bug to
> Avikalpa for now, as he already have some domain knowledge for this. I will
> file a new bug tomorrow about adding a new test to this file and flag you on
> that one, if you're interested.

Yes, no problem ;)

Comment 6

[Avikalpa Kundu [:kalpa]]

Attachment: Work in progress

How to convert nsCString to a RootedValueArray?

I need to convert the string key to a rootedvaluearray so that I can take the snapshot as

JS_CallFunctionName(cx.GetJSContext(), snapshotObj, "snapshot", Key_as_a_JSHandleValueArray , &histogram)

Comment 7

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #6)
> Created attachment 8853218 [details] [diff] [review]
> Work in progress
> 
> How to convert nsCString to a RootedValueArray?
> 
> I need to convert the string key to a rootedvaluearray so that I can take
> the snapshot as
> 
> JS_CallFunctionName(cx.GetJSContext(), snapshotObj, "snapshot",
> Key_as_a_JSHandleValueArray , &histogram)

Why do you want to do that? In your patch, you're already taking a snapshot of the keyed histograms using the |GetKeyedHistogramSnapshots| function.

Or did I misunderstand what you're trying to do?

Comment 8

[Avikalpa Kundu [:kalpa]]

(In reply to Alessio Placitelli [:Dexter] from comment #7)
> (In reply to Avikalpa Kundu [:kalpa] from comment #6)
> > Created attachment 8853218 [details] [diff] [review]
> > Work in progress
> > 
> > How to convert nsCString to a RootedValueArray?
> > 
> > I need to convert the string key to a rootedvaluearray so that I can take
> > the snapshot as
> > 
> > JS_CallFunctionName(cx.GetJSContext(), snapshotObj, "snapshot",
> > Key_as_a_JSHandleValueArray , &histogram)
> 
> Why do you want to do that? In your patch, you're already taking a snapshot
> of the keyed histograms using the |GetKeyedHistogramSnapshots| function.
> 
> Or did I misunderstand what you're trying to do?

As I understand it, I need to call the keyedhistogram with the specified key, right?

If it's the case I need to |snapshot(kExpectedKey)| to get the histogram instead of |GetKeyedHistogramSnapshots|.

Or,

If I do |GetKeyedHistogramSnapshots| how will I test the |key| and the |value|?(or |sum| whichever is applicable)

Thanks.

Comment 9

[Alessio Placitelli [:Dexter]]

(In reply to Avikalpa Kundu [:kalpa] from comment #8)
> (In reply to Alessio Placitelli [:Dexter] from comment #7)
> > (In reply to Avikalpa Kundu [:kalpa] from comment #6)
> > > Created attachment 8853218 [details] [diff] [review]
> > > Work in progress
> > > 
> > > How to convert nsCString to a RootedValueArray?
> > > 
> > > I need to convert the string key to a rootedvaluearray so that I can take
> > > the snapshot as
> > > 
> > > JS_CallFunctionName(cx.GetJSContext(), snapshotObj, "snapshot",
> > > Key_as_a_JSHandleValueArray , &histogram)
> > 
> > Why do you want to do that? In your patch, you're already taking a snapshot
> > of the keyed histograms using the |GetKeyedHistogramSnapshots| function.
> > 
> > Or did I misunderstand what you're trying to do?
> 
> As I understand it, I need to call the keyedhistogram with the specified
> key, right?

Yes, adding or getting data from a Keyed Histogram requires a key (which is a string)

> If it's the case I need to |snapshot(kExpectedKey)| to get the histogram
> instead of |GetKeyedHistogramSnapshots|.
>
> Or,
> 
> If I do |GetKeyedHistogramSnapshots| how will I test the |key| and the
> |value|?(or |sum| whichever is applicable)
> 
> Thanks.

Taking a snapshot means returning a frozen view of the data contained in the histogram at the moment the snapshotting happened, so you don't need to do anything differently for that part, as far as I can tell.

You just need to weak a little bit the way you're accessing the content of the snapshot. Just consider that, for non-keyed histograms, you had this structure:

> "HISTOGRAM_NAME": {
>   ... data ...
> }

while with keyed histograms, you have this:

> "HISTOGRAM_NAME": {
>   "key1": {
>     ... data ...
>   },
>   "key2": {
>     ... data ...
>   }
>  }

It means that, after taking the snapshot, you need to have one more |JS_GetProperty| call:

- You have one to get the histogram from the snapshot given the name.
- One to get the key from the histogram object.
- One to get the "sum"/"value".

Comment 10

[Avikalpa Kundu [:kalpa]]

Attachment: 1345861.patch

I can't understand how to |JS_GetProperty| the JSON data attached to the |key|.

I tried looking into [1] for reference but couldn't figure it out in the end.

[1] http://searchfox.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js#560,594,608

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8856127 [details] [diff] [review]
1345861.patch

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

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +90,5 @@
> +    << "Cannot fetch histogram";
> +
> +  // Get |key| and |sum| from histogram
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS::RootedObject kExpectedKeyObject(cx.GetJSContext());

nit: the "k" prefix is used for constants and this is not a constant. You should probably call this |expectedKeyData|.

@@ +94,5 @@
> +  JS::RootedObject kExpectedKeyObject(cx.GetJSContext());
> +  JS::RootedValue sum(cx.GetJSContext());
> +  
> +  // PROBLEM RIGHT HERE
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, kExpectedKey, &kExpectedKeyObject))

By looking at the docs for JS_GetProperty (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JS_GetProperty), you can see that the third parameter is a |const char*|, not a |const CString|, and that's why it fails to compile.

Same for the fourth: |expectedKeyData| must be a RootValue, not a RootObject: you can always get an object after this call using |expectedKeyData.toObject()|.

Comment 12

[Avikalpa Kundu [:kalpa]]

Attachment: 1345861.patch

(In reply to Alessio Placitelli [:Dexter] from comment #11)
> @@ +90,5 @@
> > +    << "Cannot fetch histogram";
> > +
> > +  // Get |key| and |sum| from histogram
> > +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> > +  JS::RootedObject kExpectedKeyObject(cx.GetJSContext());
> 
> nit: the "k" prefix is used for constants and this is not a constant. You
> should probably call this |expectedKeyData|.
> 
> @@ +94,5 @@
> > +  JS::RootedObject kExpectedKeyObject(cx.GetJSContext());
> > +  JS::RootedValue sum(cx.GetJSContext());
> > +  
> > +  // PROBLEM RIGHT HERE
> > +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, kExpectedKey, &kExpectedKeyObject))
> 
> By looking at the docs for JS_GetProperty
> (https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/
> JSAPI_reference/JS_GetProperty), you can see that the third parameter is a
> |const char*|, not a |const CString|, and that's why it fails to compile.
> 
> Same for the fourth: |expectedKeyData| must be a RootValue, not a
> RootObject: you can always get an object after this call using
> |expectedKeyData.toObject()|.


Thanks :)
This solves everything.

Comment 13

[Alessio Placitelli [:Dexter]]

Comment on attachment 8856588 [details] [diff] [review]
1345861.patch

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

Thank you Avikalpa, this looks good! I'm holding back the r+ just for the few nits below that needs to be addressed.

As you probably noticed, this patch is basically reusing the majority of the very same code from your first test. This suggests that we can probably do some refactoring and provide the shared functionalities between the two tests as utility functions. I can spot at least 2 potentially refactoring points:

- We could have a function that gets an histogram and clears it. We could call it |GetAndClearHistogram|.
- We could have a function that takes a snapshot of the histograms and returns a copy of the histogram data (e.g. |GetHistogramSnapshot|)

This doesn't need to happen in this patch. Would you be interested in doing this in a follow-up bug?

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +56,5 @@
> +TEST_F(TelemetryTestFixture, AccumulateKeyedCountHistogram)
> +{
> +  const uint32_t  kExpectedValue = 100;
> +  const char*     kExpectedKey   = "sample";
> +  const nsCString kKey           = NS_LITERAL_CSTRING("sample");

If |kKey| and |kExpectedKey| are only used once, there's no real benefit in declaring them here. Just use them where needed.

Moreover, please keep indentation simple: there's no need for using more than one whitespace and to enforce alignment here :)

@@ +74,5 @@
> +  JS::RootedObject testHistogramObj(cx.GetJSContext(), &testHistogram.toObject());
> +  ASSERT_TRUE(JS_CallFunctionName(cx.GetJSContext(), testHistogramObj, "clear",
> +    JS::HandleValueArray::empty(), &rval)) << "Cannot clear histogram";
> +
> +  // Accumulate in the histogram

nit: "Accumulate data in the provided key within the histogram"

@@ +86,5 @@
> +  // Get the histogram from the snapshot
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  JS::RootedObject snapshotObj(cx.GetJSContext(), &snapshot.toObject());
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), snapshotObj, "TELEMETRY_TEST_KEYED_COUNT", &histogram))
> +    << "Cannot fetch histogram";

nit: "Cannot find the expected histogram".

@@ +92,5 @@
> +  // Get histogram from keyed data
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS::RootedValue expectedKeyData(cx.GetJSContext());
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, kExpectedKey, &expectedKeyData))
> +    << "Cannot fetch key of expected keyedHistogram";

nit: "Cannot find the expected key in the histogram data"

@@ +98,5 @@
> +  // Get sum from keyed data
> +  JS::RootedObject expectedKeyDataObj(cx.GetJSContext(), &expectedKeyData.toObject());
> +  JS::RootedValue sum(cx.GetJSContext());
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), expectedKeyDataObj, "sum", &sum))
> +    << "Cannot fetch sum of expected keyedHistogram";

nit: "Cannot find the 'sum' property in the data for this key"

Comment 14

[Avikalpa Kundu [:kalpa]]

Attachment: 1345861.patch

I've corrected the 'nits' as requested.
Thanks.

(In reply to Alessio Placitelli [:Dexter] from comment #13)
> This doesn't need to happen in this patch. Would you be interested in doing
> this in a follow-up bug?

Thanks, that will be helpful.

Comment 15

[Alessio Placitelli [:Dexter]]

Comment on attachment 8856957 [details] [diff] [review]
1345861.patch

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

This looks good, with the changes below addressed.

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +74,5 @@
> +    JS::HandleValueArray::empty(), &rval)) << "Cannot clear histogram";
> +
> +  // Accumulate data in the provided key within the histogram
> +  const nsCString kKey = NS_LITERAL_CSTRING("sample");
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_KEYED_COUNT, kKey, kExpectedValue);

You can simply use NS_LITERAL_CSTRING("sample") here, instead of declaring kKey.

@@ +91,5 @@
> +  // Get histogram from keyed data
> +  JS::RootedObject histogramObj(cx.GetJSContext(), &histogram.toObject());
> +  JS::RootedValue expectedKeyData(cx.GetJSContext());
> +  const char* kExpectedKey = "sample";
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), histogramObj, kExpectedKey, &expectedKeyData))

You can simply use "sample" here. You don't need to declare "kExpectedKey".

Comment 16

[Avikalpa Kundu [:kalpa]]

Attachment: 1345861.patch

Comment 17

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857019 [details] [diff] [review]
1345861.patch

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

::: toolkit/components/telemetry/tests/gtest/TestHistograms.cpp
@@ +73,5 @@
> +  ASSERT_TRUE(JS_CallFunctionName(cx.GetJSContext(), testHistogramObj, "clear",
> +    JS::HandleValueArray::empty(), &rval)) << "Cannot clear histogram";
> +
> +  // Accumulate data in the provided key within the histogram
> +  Telemetry::Accumulate(Telemetry::TELEMETRY_TEST_KEYED_COUNT, NS_LITERAL_CSTRING("sample"), kExpectedValue);

Please break this line after |NS_LITERAL_CSTRING("sample"),| and align kExpectedValue to |Telemetry::TELEMETRY_TEST_KEYED_COUNT| on the next line.

@@ +83,5 @@
> +
> +  // Get the histogram from the snapshot
> +  JS::RootedValue histogram(cx.GetJSContext());
> +  JS::RootedObject snapshotObj(cx.GetJSContext(), &snapshot.toObject());
> +  ASSERT_TRUE(JS_GetProperty(cx.GetJSContext(), snapshotObj, "TELEMETRY_TEST_KEYED_COUNT", &histogram))

Please break this line after |"TELEMETRY_TEST_KEYED_COUNT",| and indent the remaining part of the function on the line below.

Comment 18

[Avikalpa Kundu [:kalpa]]

Attachment: 1345861.patch

Comment 19

[Alessio Placitelli [:Dexter]]

Comment on attachment 8857060 [details] [diff] [review]
1345861.patch

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

Looks good now, thanks.

Comment 20

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fa5234a41608d38eab10497c204033f4701f494

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/941929d3841e
Add C++ test coverage for the Keyed Count Histogram. r=Dexter

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/941929d3841e