Closed Bug 1278920 Opened 8 years ago Closed 7 years ago

Remove or migrate payload.simpleMeasurements.js

Categories

(Toolkit :: Telemetry, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: gfritzsche, Assigned: janerik)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [measurement:client])

Attachments

(1 file, 1 obsolete file)

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
Jason, do you know if anyone from the JS team uses this?
Flags: needinfo?(jorendorff)
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)
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.
Priority: P3 → P2
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: nobody → jrediger
Priority: P2 → P1
Attached patch Remove payload.simpleMeasurements.js (obsolete) (deleted) — Splinter Review
`getJSEngineTelemetryValue` was changed to return nothing in Bug 1409187 already.
Attachment #8964550 - Flags: review?(gfritzsche)
Blocks: 1438873
Flags: needinfo?(jorendorff) → needinfo?(jwalden+bmo)
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+
`getJSEngineTelemetryValue` was changed to return nothing in Bug 1409187 already.
Attachment #8964550 - Attachment is obsolete: true
Attachment #8965627 - Flags: review+
Attachment #8965627 - Flags: review+
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)
Attachment #8965627 - Flags: review?(alessio.placitelli)
Attachment #8965627 - Flags: review?(alessio.placitelli) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5db230cb9ff8 Remove payload.simpleMeasurements.js. r=dexter
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: