Attach system data to profiler recording

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: jsantell, Assigned: jsantell)

Tracking

(Blocks 1 bug)

41 Branch
Firefox 43
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox43 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Ccing some Framework peeps here too.

I want platform/version information (build dates/numbers for nightlies) in recordings so if someone sends us a recording profile with some weirdness, we can understand where it came from. Maybe some other info from `about:buildconfig`

* Gecko version
* Platform (b2g, fennec, firefox, etc.)
* Revision/Date of build for non-release
* OS

Bonus points for system information like RAM.

I'm guessing this should be a part of the root actor, and while we probably want to stick with feature detection vs platform detection, this helps in debugging, and will pave the way for platform specific features (like offering different tools for b2g)
A timestamp/date of recording would be useful as well
This information is already available from the device actor. You just need to send it a getDescription request to get all of that and more.
Oh, perfect; thanks Panos!
Depends on: 1172180
Good to get this in sooner than later for compat reasons.
Assignee: nobody → jsantell
Status: NEW → ASSIGNED
Original try looks good, again with more platforms incase there is some scenario where tests fail because system info is not available. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c932d1134af
Attachment #8653107 - Flags: review?(vporof) → review+
The SDK uses some XPCOM components that do not exist/work on linux according to try; this wraps them in a catch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf41ff09b8e6
Attachment #8653107 - Attachment is obsolete: true
Attachment #8654651 - Flags: review?(dtownsend)
Comment on attachment 8654651 [details] [diff] [review]
1171488-system-info-in-recordings.patch

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

::: addon-sdk/source/lib/node/os.js
@@ +16,5 @@
>  const endianness = ((new Uint32Array((new Uint8Array([1,2,3,4])).buffer))[0] === 0x04030201) ? 'LE' : 'BE';
>  
> +XPCOMUtils.defineLazyGetter(this, "oscpu", () => {
> +  try {
> +    return Cc["@mozilla.org/network/protocol;1?name=http"].getService(Ci.nsIHttpProtocolHandler).oscpu;

I'd be surprised if this was throwing unless it was happening in an xpcshell test run. I tested on my linux machine and it returns "Linux x86_64" just fine. If you really need to return null from here then please update the use of oscpu later in the code to handle that case somewhat sanely.

@@ +25,5 @@
> +
> +XPCOMUtils.defineLazyGetter(this, "hostname", () => {
> +  try {
> +    // On some platforms (Linux according to try), this service does not exist and fails.
> +    return Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService).myHostName;

Likewise this is working on my linux box but I can believe that it fails in certain configurations. Looking at node's implementation it seems that hostname will always return a string of some kind so rather than falling back to null I suggest either "" or "localhost"
Attachment #8654651 - Flags: review?(dtownsend) → review-
This'll keep it consistent then atleast. Must be some configurations result in these components not being available, atleast on try.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84d49ab53e3d
Attachment #8654651 - Attachment is obsolete: true
Attachment #8655113 - Flags: review?(dtownsend)
Comment on attachment 8655113 [details] [diff] [review]
1171488-system-info-in-recordings.patch

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

r=me for the os.js change
Attachment #8655113 - Flags: review?(dtownsend) → review+
s/non-e10s/e10s on that last one, sorry
Looks like accessing profile location fails in child process? Easy work around https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccd77ec99b94
https://hg.mozilla.org/mozilla-central/rev/619a307029d6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Experimentally backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/67ce03afe256 on suspicion of being the cause of bug 1203804
Blocks: 1203804
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
As the hyperactive pulsebot says, it wasn't at fault, relanded.
No longer blocks: 1203804
hey jordan, i wanted to reland this on fx-team but it cause conflicts in

renamed 1171488 -> 1171488-system-info-in-recordings.patch
applying 1171488-system-info-in-recordings.patch
patching file toolkit/devtools/server/tests/browser/browser_perf-recording-actor-01.js
Hunk #1 FAILED at 43
1 out of 1 hunks FAILED -- saving rejects to file toolkit/devtools/server/tests/browser/browser_perf-recording-actor-01.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1171488-system-info-in-recordings.patch

so you might want to take a look before relanding
Flags: needinfo?(jsantell)
Rebased, testing on try once more for good measure https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8cf44b5781
Attachment #8655113 - Attachment is obsolete: true
Flags: needinfo?(jsantell)
Attachment #8660048 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/a27897b90911
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.