telemetry.js: Rename all log* methods

RESOLVED FIXED in Firefox 62

Status

enhancement
P2
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: miker, Assigned: miker)

Tracking

57 Branch
Firefox 62
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

log: calls T.getHistogramById().add()
logKeyed: calls T.getKeyedHistogramById().add()
logScalar: calls T.scalarSet()
logCountScalar: calls T.scalarAdd()
logKeyedScalar: calls T.keyedScalarAdd()
    
All the low level methods are renamed to match the real method names from Services.telemetry. Example: log is replaced by getHistogramById. The caller is responsible for calling add() afterwards. In case the histogram doesn't exist, we can return a mock { add: () => {} }.
Has Regression Range: --- → irrelevant
Has STR: --- → irrelevant
logOncePerBrowserVersion: complex method to check if a probe was already logged, ends up calling T.getHistogramById().add()
Creating `add: () => {}` for undefined histograms would mask bugs so we will not be doing this.
Assignee: nobody → mratcliffe
Status: NEW → ASSIGNED
Attachment #8973163 - Attachment is obsolete: true

Comment 7

a year ago
mozreview-review
Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

https://reviewboard.mozilla.org/r/241686/#review247594

That's great! Thanks a lot for the cleanup Mike.

::: devtools/client/shared/telemetry.js:286
(Diff revision 1)
> -      return;
> +
> +    if (histogramId) {
> +      try {
> +        histogram = Services.telemetry.getHistogramById(histogramId);
> +      } catch (e) {
> +        dump(`Warning: An attempt was made to write to the ${histogramId} ` +

I don't feel strongly about this, but if we want to avoid the caller to throw when calling ".add()" on this histogram, we could return a mock 
  histogram = { add : () => {} };
  
Up to you. Same comment for getKeyedHistogramById
Attachment #8973193 - Flags: review?(jdescottes) → review+
(In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from comment #2)
> Creating `add: () => {}` for undefined histograms would mask bugs so we will
> not be doing this.

Had not seen this comment. I don't really follow the reasoning, the current try/catch around the whole method has the exact same impact.
(In reply to Julian Descottes [:jdescottes][:julian] from comment #8)
> (In reply to Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] from
> comment #2)
> > Creating `add: () => {}` for undefined histograms would mask bugs so we will
> > not be doing this.
> 
> Had not seen this comment. I don't really follow the reasoning, the current
> try/catch around the whole method has the exact same impact.

Ah, you mean to stop failed telemetry breaking stuff... I will change all methods to prevent throwing but dump a warning.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d555b3a047d1
telemetry.js: Rename all log* methods r=jdescottes
https://hg.mozilla.org/mozilla-central/rev/d555b3a047d1
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.