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

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Varun [he/him IST]]

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

Comment 1

[Chris H-C :chutten]

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?

Comment 2

[Varun [he/him IST]]

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

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

Comment 3

[Varun [he/him IST]]

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.

Comment 4

[Chris H-C :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.

Comment 5

[Varun [he/him IST]]

Attachment: Bug 1524227 Replacing getParentProcessScalar with generic getProcessScalar

Replacing existing getParentProcessScalars with a generic implementation of getProcessScalars

Comment 6

[Chris H-C :chutten]

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?

Comment 7

[Chris H-C :chutten]

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

Comment 8

[Chris H-C :chutten]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d71d8276a4b4c128baac93ae4fc0d52de0be7e62

Comment 9

[Chris H-C :chutten]

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

Comment 10

[Chris H-C :chutten]

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 https://devtools-html-slack.herokuapp.com/

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!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/55f7952d947c
Replacing getParentProcessScalar with generic getProcessScalar r=chutten

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/55f7952d947c