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

RESOLVED FIXED in Firefox 67

Status

()

enhancement
P3
normal
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: vdey, Assigned: vdey, Mentored)

Tracking

66 Branch
mozilla67
Points:
---

Firefox Tracking Flags

(firefox67 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

5 months ago

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

Comment 1

5 months ago

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
Status: UNCONFIRMED → ASSIGNED
Component: Untriaged → Telemetry
Ever confirmed: true
Priority: -- → P3
Product: Firefox → Toolkit
Assignee

Comment 2

5 months ago

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

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

Assignee

Comment 3

4 months ago

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)

Comment 4

4 months ago

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)
Assignee

Comment 5

4 months ago

Replacing existing getParentProcessScalars with a generic implementation of getProcessScalars

Comment 6

4 months ago

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

4 months ago

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

Comment 9

4 months ago

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.

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

4 months ago
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/55f7952d947c
Replacing getParentProcessScalar with generic getProcessScalar r=chutten

Comment 12

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
You need to log in before you can comment on or make changes to this bug.