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

RESOLVED FIXED in Firefox 61

Status

()

P3
normal
RESOLVED FIXED
9 months ago
7 months ago

People

(Reporter: adbugger, Assigned: adbugger, Mentored)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

9 months ago
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.

Updated

8 months ago
Mentor: chutten
Priority: -- → P3

Comment 1

8 months ago
:adbugger, How about this one? Are you sill working on this? Or shall we unassign it for now?
Flags: needinfo?(adibhar97)
(Assignee)

Comment 2

8 months ago
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)
(Assignee)

Comment 3

8 months ago
Created attachment 8959452 [details] [diff] [review]
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.
Attachment #8959452 - Flags: review?(chutten)

Comment 4

8 months ago
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)
(Assignee)

Comment 5

8 months ago
Created attachment 8961578 [details] [diff] [review]
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.
Attachment #8959452 - Attachment is obsolete: true
Attachment #8961578 - Flags: review?(chutten)

Comment 6

8 months ago
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+
(Assignee)

Comment 7

8 months ago
Created attachment 8961999 [details] [diff] [review]
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
Attachment #8961578 - Attachment is obsolete: true
Attachment #8961999 - Flags: review?(chutten)

Comment 8

8 months ago
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+

Updated

8 months ago
Keywords: checkin-needed

Comment 9

8 months ago
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfa48d6c1c6c
Refactor internal_JSKeyedHistogram_Add r=chutten
Keywords: checkin-needed

Comment 10

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bfa48d6c1c6c
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
(Assignee)

Comment 11

7 months ago
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)

Comment 12

7 months ago
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)
(Assignee)

Comment 13

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

Comment 14

7 months ago
Created attachment 8971856 [details] [diff] [review]
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
Attachment #8971856 - Flags: review?(chutten)

Comment 15

7 months ago
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+

Comment 16

7 months ago
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?
(Assignee)

Comment 17

7 months ago
Sure, I'll do that.
(Assignee)

Updated

7 months ago
See Also: → bug 1457883

Comment 18

7 months ago
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.