1442667 - Refactor internal_JSKeyedHistogram_Add to use the type checking utility function introduced in bug 1428893

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Aditya Bharti [:adbugger]]

In bug 1428893, the internal_JSHistogram_Add function was extended to support arrays of values. Since the type checking code was the same for the singular or plural case, it was pulled out into its own separate function internal_JSHistogram_CoerceValue. The keyed histogram can make use of the same type checking utility function in its code.

Comment 1

[Chris H-C :chutten]

:adbugger, How about this one? Are you sill working on this? Or shall we unassign it for now?

Comment 2

[Aditya Bharti [:adbugger]]

Sorry, I've been a little side tracked past few days. Yes I'm still working on this. I'll upload a patch by the end of today.

Comment 3

[Aditya Bharti [:adbugger]]

Attachment: 1442667_v1.diff

Initial patch. Not extending support for arrays, so I haven't added any tests. Both gtests Telemetry* and xpcshell-test test_TelemetryHistograms.js pass.

Comment 4

[Chris H-C :chutten]

Comment on attachment 8959452 [details] [diff] [review]
1442667_v1.diff

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

The code coverage report[1] can give you an idea what parts of the original implementation are currently untested (the lines highlighted red aren't tested). So, if you can, I think we would benefit from a few new test cases to cover those error conditions (0 argument case, invalid categorical label string, unconvertable numeric argument  (this last one is impossible because of the earlier isNumber || isBoolean check)) which would make absolutely certain that the new code behaves like the old code.

[1]: https://codecov.io/gh/marco-c/gecko-dev/src/master/toolkit/components/telemetry/TelemetryHistogram.cpp#L1543

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1605,2 @@
>  
> +  uint32_t value = 0;

We're not using this for a few lines, so maybe pop it down to line 1612 or so, closer to where it's used?

Comment 5

[Aditya Bharti [:adbugger]]

Attachment: 1442667_v2.patch

Added tests to improve code coverage. Tested for invalid string parameter and no argument. The last test case of failing to convert numeric argument does seem impossible because of the aforementioned check.

xpcshell-tests and gtests Telemetry* pass locally. Here is the try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ed281e555d992ee5c05b77124ff669fa30558b2

I had to retrigger the gecko decision task for some reason. It failed the first time.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8961578 [details] [diff] [review]
1442667_v2.patch

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

Remove these two comments and then we can get this checked in.

::: toolkit/components/telemetry/tests/unit/test_TelemetryHistograms.js
@@ +1132,5 @@
> +  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_LINEAR");
> +  h.clear();
> +
> +  // Call add with no arguments.
> +  // Should return true with no accumulation, and print an error to console.

This is what the C++ code is doing, but not what the test is testing, so I think this comment will confuse future readers of this test.

@@ +1147,5 @@
> +  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_CATEGORICAL");
> +  h.clear();
> +
> +  // Call add with a key and an invalid label.
> +  // Should return true with no accumulation, and print an error to console.

Same here

Comment 7

[Aditya Bharti [:adbugger]]

Attachment: 1442667_v3.patch

Sorry it took so long. I lost my previous changes after a merge. Had to go back one revision and re build.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=393f2d8eebf34e21c8f9f4438e5eb01d6c3c8ba5

Comment 8

[Chris H-C :chutten]

Comment on attachment 8961999 [details] [diff] [review]
1442667_v3.patch

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

Looks good to me, thank you for this refactor!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa48d6c1c6c
Refactor internal_JSKeyedHistogram_Add r=chutten

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/bfa48d6c1c6c

Comment 11

[Aditya Bharti [:adbugger]]

Comment on attachment 8961999 [details] [diff] [review]
1442667_v3.patch

># HG changeset patch
># User Aditya Bharti <[e-mail]>
># Date 1521867569 -19800
>#      Sat Mar 24 10:29:29 2018 +0530
># Node ID 28cb155b83a2bc41194f289af0f63b46dc442e6d
># Parent  5bc910c1f477d8095dcde2c96b7e8639211b9a84
>bug 1442667 - Refactor internal_JSKeyedHistogram_Add r?chutten

>+add_task(async function test_keyed_no_arguments() {
>+  // Test for no accumulation when add is called with no arguments
>+  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_LINEAR");
>+  h.clear();
>+
>+  h.add();
>+
>+  let s = h.snapshot();
>+  // Snapshot property sum is undefined until the first accumulation.
>+  Assert.equal(s.sum, undefined);
>+});

>+add_task(async function test_keyed_categorical_invalid_string() {
>+  // Test for no accumulation when add is called on a
>+  // keyed categorical histogram with an invalid string label.
>+  let h = Telemetry.getKeyedHistogramById("TELEMETRY_TEST_KEYED_CATEGORICAL");
>+  h.clear();
>+
>+  h.add("someKey", "#notALablel");
>+
>+  let s = h.snapshot();
>+  // Snapshot property sum is undefined until the first accumulation.
>+  Assert.equal(s.sum, undefined);
>+});

I was revisiting this piece of code for another bug and I think I found an error. The lines capturing the snapshot should be h.snapshot(key). 
For the no_arguments case, I'm not sure how we'd check for no accumulations since we're not giving a key.
For the invalid string case, I need to modify the code and change the line which gets the snapshot.
Without the key, snapshot.sum property will always return undefined and the tests will always pass.

While it is a simple fix, I don't know what the procedure is since this has already been pushed. I don't _think_ this needs to be backed out right now since it's not breaking anything. I'm needinfoing for further information on how to proceed.

Apologies for messing this up.

Comment 12

[Chris H-C :chutten]

No worries, we can handle this here and now. Thank you for catching this!

What we can do to test the 0-argument for keyed case is to clear the keyed histogram, call add on it without a key, and then assert that there are no keys in it.

Make sense?

Comment 13

[Aditya Bharti [:adbugger]]

Yup. That makes perfect sense. I didn't know clearing a histogram also removes keys. I'll get to it in a few hours.

Comment 14

[Aditya Bharti [:adbugger]]

Attachment: Corrected test cases

Sorry it took so long.

This patch is based off the previous patch and the latest mc merge. Landability at least should not be an issue.

Try push (just to be safe, tested locally with gtest and xpcshell-tests):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=21024f310c972feac6443079ff7177109d264d8d

Comment 15

[Chris H-C :chutten]

Comment on attachment 8971856 [details] [diff] [review]
Corrected test cases

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

Looks good.

Comment 16

[Chris H-C :chutten]

I think to make this easier to understand if people happen upon it in the future, we should file a bug specifically for this. My r+ can carry over and you can mark it checkin-needed right away.

Does that sound like a plan, :adbugger?

Comment 17

[Aditya Bharti [:adbugger]]

Sure, I'll do that.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaaf24c64666
Refactor internal_JSKeyedHistogram_Add r=chutten

Comment 19

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/aaaf24c64666