Closed Bug 1442664 Opened 2 years ago Closed 2 years ago

Extend the JSKeyedHistogram APIs to allow Javascript to accumulate multiple samples to a keyed histogram in one call

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, 4 obsolete files)

In bug 1364043, the C++ Telemetry::Accumulate function was extended to allow adding multiple samples to a histogram in one call.

Bug 1428885 implemented the functionality for keyed histograms.

Bug 1428888 implemented the same functionality for categorical histograms.

Bug 1428893 extended the JSHistogram API to provide the same functionality.

This bug is for extending the JSKeyedHistogram APIs so that the same functionality is available through JS probes. See comment 27 on bug 1364043 for more information.
Mentor: chutten
Priority: -- → P3
:adbugger, are you sill working on this? Or shall we unassign it for now?
Flags: needinfo?(adibhar97)
:chutten, sorry about the absence. I'm working on another bug at the moment, and I've a few other commitments this weekend. I'll be able to take a look at this only on Monday. I'm un-assigning it for now and I'll get back to it then if no one has taken it up yet.
Flags: needinfo?(adibhar97)
Assignee: adibhar97 → nobody
Attached patch Modified JSKeyedHistogram_Add (obsolete) — Splinter Review
I modified the internal_JSKeyedHistogram_Add function to support accumulating an array of values. For now, all tests pass, so there are no regressions.

I'll write some new test cases in the next version of this patch. I'll start with replicating all the test cases that were added in bug 1428893.
Assignee: nobody → adibhar97
Status: NEW → ASSIGNED
Attachment #8963991 - Flags: review?(chutten)
Comment on attachment 8963991 [details] [diff] [review]
Modified JSKeyedHistogram_Add

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

Looks like a solid start.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1639,5 @@
> +            NS_ConvertUTF8toUTF16(nsPrintfCString("Element at index %d failed type checks", arrayIdx)));
> +        return true;
> +      }
> +      values.AppendElement(value);
> +    }

This is all looking very similar. Can we factor the "collect values from JS Array" code out and reuse it between the keyed and unkeyed cases?
Attachment #8963991 - Flags: review?(chutten)
I've pulled the common code out into a single function that handles extraction of the value array for keyed, non-keyed histograms with single and multiple values. 

I've gone ahead and modified the internal_JSHistogram_Add function to use the new utility function. I realize only now that that change probably belongs in a separate bug. 

In any case, here is the next version of the patch. If everything is fine, I'll get started with the tests (and revert the internal_JSHistogram_Add function, if required).
Attachment #8963991 - Attachment is obsolete: true
Attachment #8965400 - Flags: review?(chutten)
Comment on attachment 8965400 [details] [diff] [review]
Added utility function internal_JSHistogram_GetValueArray to extract arguments from CallArgs

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

The refactor of the non-keyed call to use the new function is fine to keep in the same bug and the same commit if that's easier.

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1190,4 @@
>    return true;
>  }
>  
> +bool internal_JSHistogram_GetValueArray(JSContext* aCx, JS::CallArgs& args, uint32_t aHistogramType, HistogramID aId,

The style in this file is to have the return type on its own line before the rest of the signature

@@ +1213,5 @@
> +    aArray.AppendElement(1);
> +    return true;
> +  }
> +
> +  if (args[firstArgIndex].isObject() && !args[firstArgIndex].isString()) {

Should we be ensuring args is at least firstArgIndex in length? What happens if args is 0-length for a keyed histogram, do we dereference off the end of the array here?
Attachment #8965400 - Flags: review?(chutten) → review-
(In reply to Chris H-C :chutten from comment #6)
 
> The style in this file is to have the return type on its own line before the
> rest of the signature

Made this change.

> Should we be ensuring args is at least firstArgIndex in length? What happens
> if args is 0-length for a keyed histogram, do we dereference off the end of
> the array here?

For 0-length args with a non-keyed histogram, the function will check for the count histogram type and fail appropriately.
For 0-length args with a keyed histogram, the keyedHistogram_Add function itself has the following check [1] which will ensure that if this function is called with keyed histogram, then we have at least one argument.

[1]: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp#1568-1571

Nevertheless, I will include this as a test to cover all my bases. I'll start working on the tests now.
(In reply to Aditya Bharti [:adbugger] from comment #7)
> Nevertheless, I will include this as a test to cover all my bases.

I forgot to mention that I tested my patch locally with the Telemetry xpcshell-tests and gtests before uploading here. So the test_keyed_no_arguments test case from tests_TelemetryHistograms.js (added in bug 1442667 to improve code coverage) has already covered this scenario.
Attached patch Test cases (obsolete) — Splinter Review
Added test cases. Tested locally with gtests and xpcshell tests.
Attachment #8965400 - Attachment is obsolete: true
Attachment #8966000 - Flags: review?(chutten)
(In reply to Aditya Bharti [:adbugger] from comment #7)
> For 0-length args with a non-keyed histogram, the function will check for
> the count histogram type and fail appropriately.
> For 0-length args with a keyed histogram, the keyedHistogram_Add function
> itself has the following check [1] which will ensure that if this function
> is called with keyed histogram, then we have at least one argument.
> 
> [1]:
> https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/
> TelemetryHistogram.cpp#1568-1571
> 
> Nevertheless, I will include this as a test to cover all my bases. I'll
> start working on the tests now.

Ah, it was further up than the context provided. Yes, a test would be wonderful, thank you.

I shall review the patch now.
Comment on attachment 8966000 [details] [diff] [review]
Test cases

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

Looks really good. Do we have tests for attempting to accumulate using non-array (and non-string) objects?
Attachment #8966000 - Flags: review?(chutten) → review+
Hey, :adbugger, are you experiencing difficulties? Just a short delay? I'm checking up on my mentored bugs and wanted to make sure you weren't stuck.
Flags: needinfo?(adibhar97)
Nope, no difficulties. Just a bunch of deadlines. I'll update the cases by tonight.
Flags: needinfo?(adibhar97)
Added a test case for non-array, non-string object. Tested locally with gtest and xpcshell tests.
Attachment #8966000 - Attachment is obsolete: true
Attachment #8969586 - Flags: review?(chutten)
Comment on attachment 8969586 [details] [diff] [review]
Test for non-array, non-string object

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

Excellent, thank you. Now all this needs is a commit message and we can get it checked in.
Attachment #8969586 - Flags: review?(chutten) → review+
Attached patch 1442664_v5.patchSplinter Review
Added commit message. Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=369c1e577529cf6eca26974a257f8323afb6322a
Attachment #8969586 - Attachment is obsolete: true
Attachment #8969779 - Flags: review?(chutten)
Comment on attachment 8969779 [details] [diff] [review]
1442664_v5.patch

Looks good, let's get this in.
Attachment #8969779 - Flags: review?(chutten) → review+
Keywords: checkin-needed
Hi,

I've received this error when trying to land this patch:

hg qpush -a
applying 1442664_v5.patch
patching file toolkit/components/telemetry/TelemetryHistogram.cpp
Hunk #2 FAILED at 1283
Hunk #3 FAILED at 1660
2 out of 3 hunks FAILED -- saving rejects to file toolkit/components/telemetry/TelemetryHistogram.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh 1442664_v5.patch

Can you please take a look?
Flags: needinfo?(adibhar97)
Keywords: checkin-needed
(In reply to Cristian Brindusan [:cristian_brindusan] from comment #18)
> Hi,
> 
> I've received this error when trying to land this patch:
> 
> hg qpush -a
> applying 1442664_v5.patch
> patching file toolkit/components/telemetry/TelemetryHistogram.cpp
> Hunk #2 FAILED at 1283
> Hunk #3 FAILED at 1660
> 2 out of 3 hunks FAILED -- saving rejects to file
> toolkit/components/telemetry/TelemetryHistogram.cpp.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh 1442664_v5.patch
> 
> Can you please take a look?

I'm still new to this (this is the first time I've encountered an error when trying to land a patch) so I'll need some help.
I checked the version of the file TelemetryHistogram.cpp here [1] and I seem to have the latest version. So its not a file version problem. Can you tell me the steps to generate some sort of a log file, or maybe some way I can see the reasons for rejection? I only have try access so I cannot push to any other repository and see the error myself.

[1]: https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/TelemetryHistogram.cpp

Meanwhile I'll try pulling the latest changes and basing my patch on that.

:chutten is there some standard way of accessing error logs for land attempts that I'm unaware of? How do I proceed here, apart from pulling the latest version?
Flags: needinfo?(chutten)
Flags: needinfo?(cbrindusan)
Flags: needinfo?(adibhar97)
Flags: needinfo?(cbrindusan) → needinfo?(aryx.bugmail)
This is the exported file based off the latest tip for mozilla central. I'm not sure if it will remove all errors but cannot hurt to try. 

Tested locally. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ebfe20c18fbe2beeda7763755a6f23bf438c75e
Attachment #8970049 - Flags: review?(chutten)
The latest version is based on an April 22 m-c merge, so it should be landable. Leaving Aryx's ni? for consideration.
Flags: needinfo?(chutten)
Comment on attachment 8970049 [details] [diff] [review]
Export based off latest tip revision

Is this ok to try to checkin this? I'd like to know if there are still some issues to be worked out, and I think a checkin attempt would be informative.
Attachment #8970049 - Flags: checkin?(chutten)
Pushed by ebalazs@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e4ba7a4aa1
JSKeyedHistogram_Add accumulates multiple samples in one call r=chutten
Attachment #8970049 - Flags: checkin?(chutten)
https://hg.mozilla.org/mozilla-central/rev/f2e4ba7a4aa1
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8970049 [details] [diff] [review]
Export based off latest tip revision

Patch landed, removing r?
Attachment #8970049 - Flags: review?(chutten)
Flags: needinfo?(aryx.bugmail)
You need to log in before you can comment on or make changes to this bug.