Remove or migrate payload.simpleMeasurements.js

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: gfritzsche, Assigned: janerik)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 years ago
We are currently reviewing our Telemetry ping contents at [1], which contain a lot of historical baggage.

One data point that we don't have a clear owner for is payload.simpleMeasurements.js (we documented it at [2]).
We should either:
* remove this if it is currently unused or
* migrate them to histograms or scalars (coming in bug 1275517), potentially as "opt-in"

[1]: https://docs.google.com/spreadsheets/d/1y5gbtHibpEcZMObKDt1AXSXM4wLtw4vwXkN10OKV7H0
[2]: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/main-ping.html#js
(Reporter)

Comment 1

3 years ago
Jason, do you know if anyone from the JS team uses this?
Flags: needinfo?(jorendorff)
(Reporter)

Updated

3 years ago
Blocks: 1201837
This is somewhat of a guess, but I think I understand that data section to be essentially a catchall location to put JS telemetry queries in, so that SpiderMonkey people have all that info in a single place.  The particular contents of it will vary over time as we choose to collect one piece of information or another.

As to the two pieces of information in there now:

* setProto I believe is explainable as us wanting to know how common [[Prototype]] mutation is.  This likely dates to the time when we were attempting to discourage this anti-pattern, in the hopes of not specifying it and ultimately removing it.  Unfortunately the terrorists won, __proto__ and Object.setPrototypeOf both got specified, and the chances of prototype mutation dying now are as close to zero as possible.  So that bit of it possibly could be removed, if we removed the corresponding data collection in SpiderMonkey.

* customIter is useful information in determining at what point we can remove support for the 2006-era SpiderMonkey iteration protocol, where .next() returned values or threw a per-realm StopIteration value to halt iteration.  This protocol has been entirely superseded by ES6's new protocol, that returns { value: ..., done: true/false } objects to the same effect.  I don't know who's looking at customIter numbers, but removing custom iteration support is definitely something we want to do still, and these numbers are necessary to have much sense of when use of __iterator__ has diminished to the point that we can remove support.

As far as migrating these values elsewhere goes, I imagine as long as they continue to be collected somehow (or possibly we remove the setProto one), no one would particularly care.  As to who *owns* this data and watches it, the answer these days might well be no one.
Flags: needinfo?(jorendorff)

Comment 3

3 years ago
Every piece of data must have somebody responsible for it and somebody signed up to actually use it. Without those we should simply remove the data collection and when it's valuable to the team you can re-add it.
(Reporter)

Updated

a year ago
Priority: P3 → P2
(Assignee)

Comment 4

a year ago
Bug 1409187 removed the last item from `simpleMeasurements.js` and was rolled out in Firefox 59.
We will now remove the remaining empty object.

We now have good alternatives for additional data collection: Use Counters[1], Scalars[2], Histograms[3].

[1]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/use-counters.html
[2]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/scalars.html
[3]: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/histograms.html

Any of these are now the preferred way to record data.
If necessary we can re-add something at any time.

Is any additional follow-up cleanup on the JS site necessary?
Flags: needinfo?(jorendorff)
(Assignee)

Updated

a year ago
Assignee: nobody → jrediger
Priority: P2 → P1
(Assignee)

Comment 5

a year ago
`getJSEngineTelemetryValue` was changed to return nothing in Bug 1409187 already.
Attachment #8964550 - Flags: review?(gfritzsche)
(Assignee)

Updated

a year ago
Blocks: 1438873
(Assignee)

Updated

a year ago
Flags: needinfo?(jorendorff) → needinfo?(jwalden+bmo)
(Reporter)

Comment 6

a year ago
Comment on attachment 8964550 [details] [diff] [review]
Remove payload.simpleMeasurements.js

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

Alessio, can you take a look?
Attachment #8964550 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment on attachment 8964550 [details] [diff] [review]
Remove payload.simpleMeasurements.js

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

This looks good, thanks! Let's wait for the JS team to follow-up before landing it.

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ -776,5 @@
>      }
>  
>      ret.startupInterrupted = Number(Services.startup.interrupted);
>  
> -    ret.js = Cu.getJSEngineTelemetryValue();

Since we're no longer using this, we might ask the JS team if that code (https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/js/xpconnect/idl/xpccomponents.idl#614) can be removed as well. It's good material for a separate patch/follow-up bug!

::: toolkit/components/telemetry/docs/data/main-ping.rst
@@ +698,5 @@
>  - Firefox 61:
>  
>    - Stopped reporting ``childPayloads`` (`bug 1443599 <https://bugzilla.mozilla.org/show_bug.cgi?id=1443599>`_).
>    - Stopped reporting ``saved-session`` pings on Firefox Desktop (`bug 1443603 <https://bugzilla.mozilla.org/show_bug.cgi?id=1443603>`_).
> +  - Stopped reporting ``simpleMeasurements.js`` (`bug 1278920 <https://bugzilla.mozilla.org/show_bug.cgi?id=1278920>`_)

super-nit: missing trailing dot '.'
Attachment #8964550 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Comment 8

a year ago
`getJSEngineTelemetryValue` was changed to return nothing in Bug 1409187 already.
(Assignee)

Updated

a year ago
Attachment #8964550 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8965627 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8965627 - Flags: review+

Comment 9

11 months ago
I'm not sure who actually is the right person for this.  But whatever code might exist on the JS engine side to service this one API -- I kind of doubt there is any, as it would have likely disappeared when removed from the getJSEngineBlah function implementation -- is probably small, and its living longer by mistake is not going to meaningfully harm us.  Carry on!
Flags: needinfo?(jwalden+bmo)
(Assignee)

Updated

11 months ago
Attachment #8965627 - Flags: review?(alessio.placitelli)
Attachment #8965627 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 10

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5db230cb9ff8
Remove payload.simpleMeasurements.js. r=dexter
Keywords: checkin-needed

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5db230cb9ff8
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
status-firefox50: affected → ---
You need to log in before you can comment on or make changes to this bug.