Closed Bug 1524227 Opened 6 years ago Closed 6 years ago

Add getProcessScalars in TelemetryTestUtils that can be used in test_TelemetryScalars.js instead of its custom method


(Toolkit :: Telemetry, enhancement, P3)

66 Branch



Tracking Status
firefox67 --- fixed


(Reporter: varundey20, Assigned: varundey20, Mentored)



(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0

Good job with the title, it's very informative and clear. The bug Description (first comment) should probably be a longer-form description of Why we think we need to do it and any other background information needed so that someone else (or yourself in the future) coming to the bug knows what it's about.

Something like

"test_TelemetryScalars.js has a method getProcessScalars that is essentially getParentProcessScalars but one step more generic. If we put this in TelemetryTestUtils we can rewrite getParentProcessScalars to be getProcessScalars("parent", ...) and then have the generic method for other consumers who want a specific, non-parent-process snapshot of scalars. (For instance, this is how a test snapshots content scalars today)"

You good to start working on this in the near future?

Assignee: nobody → varundey20
Mentor: chutten
Component: Untriaged → Telemetry
Ever confirmed: true
Priority: -- → P3
Product: Firefox → Toolkit

Argh! Don't know why it didn't seem obvious to me while creating this bug.

And yes, I will start working on this. :)

Since getParentProcessScalars takes args aChannel, aKeyed = false, aClear = false in order and getProcessScalars takes aProcessName, aKeyed = false, aClear = false, we would need to add one more parameter in the generic refactored getProcessScalars which would satisfy aChannel. It will look like this after that: getProcessScalars(aProcessName, aChannel, aKeyed = false, aClear = false). Currently we are passing Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN as aKeyed, is it okay to use this constant in the generic method? The caller will call like this after that : TelemetryTestUtils.getProcessScalars("dynamic", Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN, false, false); and TelemetryTestUtils.getProcessScalars("parent", Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN);. Basically in the first example, the second argument is serving no purpose other than to satisfy the function call signature.

Flags: needinfo?(chutten)

We should adopt the canRecordExtended manipulations from getParentProcessScalars in the new getProcessScalars. It shouldn't matter so much anymore except for ~2 internal tests, but it really matters for those tests.

Since no one (except those tests) actually wants to change aChannel, let's put it as the last argument to these functions and give it a default of Ci.nsITelemetry.DATASET_RELEASE_CHANNEL_OPTIN. That'll improve the ergonomics of the API, encouraging people to use it.

It'll mean changing existing call sites of TelemetryTestUtils.getParentProcessScalars which will make for a bigger patch. If you're not comfortable doing it all at once we can leave the order for now.

Flags: needinfo?(chutten)

Replacing existing getParentProcessScalars with a generic implementation of getProcessScalars

The try push is coming up with a bunch of what appear to be unrelated errors. I encountered them in my recent pushes as well, but updating to the latest top of the tree helped out.

Could you rebase your changes onto the top of the tree so we can try again?

Actually, nevermind, I can do that myself. No need to force you to do it :)

Incidentally the "unrelated errors" had to do with some certificates expiring, making it impossible to make TLS connections to, say, It's been resolved across the tree and the new try push is green. I'll go ahead and push this to autoland.

I would like to work on Firefox debugger or any other React project which Firefox is using. :)

Unfortunately I don't know first-hand of any React bugs to work on. Luckily when I asked around everyone was very happy to tell me about theirs :)

You mentioned the debugger. You mean the devtools debugger? They track their issues on github. You can find their list of good first issues here. If you go this route, definitely join the devtools slack

If you're looking for something bigger, we're looking to convert out performance-test-monitoring site perfherder to React. There's a meta bug breaking down the components here: bug 1450044. This will require a lot more understanding of how things are currently written and software architecture design.

Hopefully that helps!

Pushed by
Replacing getParentProcessScalar with generic getProcessScalar r=chutten
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.