Closed Bug 1442667 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Telemetry, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: adbugger, Assigned: adbugger, Mentored)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Mentor: chutten
Priority: -- → P3
:adbugger, How about this one? Are you sill working on this? Or shall we unassign it for now?
Flags: needinfo?(adibhar97)
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.
Flags: needinfo?(adibhar97)
Attached patch 1442667_v1.diff (obsolete) — Splinter Review
Initial patch. Not extending support for arrays, so I haven't added any tests. Both gtests Telemetry* and xpcshell-test test_TelemetryHistograms.js pass.
Attachment #8959452 - Flags: review?(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?
Attachment #8959452 - Flags: review?(chutten)
Attached patch 1442667_v2.patch (obsolete) — Splinter Review
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.
Attachment #8959452 - Attachment is obsolete: true
Attachment #8961578 - Flags: review?(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
Attachment #8961578 - Flags: review?(chutten) → review+
Attached patch 1442667_v3.patchSplinter Review
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
Attachment #8961578 - Attachment is obsolete: true
Attachment #8961999 - Flags: review?(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!
Attachment #8961999 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa48d6c1c6c
Refactor internal_JSKeyedHistogram_Add r=chutten
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bfa48d6c1c6c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8961999 [details] [diff] [review]
1442667_v3.patch

># HG changeset patch
># User Aditya Bharti <adibhar97@gmail.com>
># 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.
Flags: needinfo?(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?
Flags: needinfo?(chutten)
Yup. That makes perfect sense. I didn't know clearing a histogram also removes keys. I'll get to it in a few hours.
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
Attachment #8971856 - Flags: review?(chutten)
Comment on attachment 8971856 [details] [diff] [review]
Corrected test cases

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

Looks good.
Attachment #8971856 - Flags: review?(chutten) → review+
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?
Sure, I'll do that.
See Also: → 1457883
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaaf24c64666
Refactor internal_JSKeyedHistogram_Add r=chutten
You need to log in before you can comment on or make changes to this bug.