Closed
Bug 1442664
Opened 6 years ago
Closed 6 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)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: adbugger, Assigned: adbugger, Mentored)
References
Details
Attachments
(2 files, 4 obsolete files)
11.80 KB,
patch
|
chutten
:
review+
|
Details | Diff | Splinter Review |
11.80 KB,
patch
|
Details | Diff | Splinter Review |
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.
Updated•6 years ago
|
Mentor: chutten
Priority: -- → P3
Comment 1•6 years ago
|
||
:adbugger, are you sill working on this? Or shall we unassign it for now?
Flags: needinfo?(adibhar97)
Assignee | ||
Comment 2•6 years ago
|
||
: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 | ||
Updated•6 years ago
|
Assignee: adibhar97 → nobody
Assignee | ||
Comment 3•6 years ago
|
||
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.
Comment 4•6 years ago
|
||
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)
Assignee | ||
Comment 5•6 years ago
|
||
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 6•6 years ago
|
||
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-
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
(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.
Assignee | ||
Comment 9•6 years ago
|
||
Added test cases. Tested locally with gtests and xpcshell tests.
Attachment #8965400 -
Attachment is obsolete: true
Attachment #8966000 -
Flags: review?(chutten)
Comment 10•6 years ago
|
||
(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 11•6 years ago
|
||
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+
Comment 12•6 years ago
|
||
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)
Assignee | ||
Comment 13•6 years ago
|
||
Nope, no difficulties. Just a bunch of deadlines. I'll update the cases by tonight.
Flags: needinfo?(adibhar97)
Assignee | ||
Comment 14•6 years ago
|
||
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 15•6 years ago
|
||
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+
Assignee | ||
Comment 16•6 years ago
|
||
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 17•6 years ago
|
||
Comment on attachment 8969779 [details] [diff] [review] 1442664_v5.patch Looks good, let's get this in.
Attachment #8969779 -
Flags: review?(chutten) → review+
Updated•6 years ago
|
Keywords: checkin-needed
Comment 18•6 years ago
|
||
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
Assignee | ||
Comment 19•6 years ago
|
||
(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)
Updated•6 years ago
|
Flags: needinfo?(cbrindusan) → needinfo?(aryx.bugmail)
Assignee | ||
Comment 20•6 years ago
|
||
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)
Comment 21•6 years ago
|
||
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)
Assignee | ||
Comment 22•6 years ago
|
||
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)
Comment 23•6 years ago
|
||
Pushed by ebalazs@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f2e4ba7a4aa1 JSKeyedHistogram_Add accumulates multiple samples in one call r=chutten
Updated•6 years ago
|
Attachment #8970049 -
Flags: checkin?(chutten)
Comment 24•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f2e4ba7a4aa1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 25•6 years ago
|
||
Comment on attachment 8970049 [details] [diff] [review] Export based off latest tip revision Patch landed, removing r?
Attachment #8970049 -
Flags: review?(chutten)
Updated•6 years ago
|
Flags: needinfo?(aryx.bugmail)
You need to log in
before you can comment on or make changes to this bug.
Description
•