1458203 - telemetry.js: Rename all log* methods

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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: () => {} }.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

logOncePerBrowserVersion: complex method to check if a probe was already logged, ends up calling T.getHistogramById().add()

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Creating `add: () => {}` for undefined histograms would mask bugs so we will not be doing this.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: telemetry.js: Rename all log* methods

Comment 4

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1987fbb1ad6b9658d85f08bacec660e06a1e75d

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1458203 - telemetry.js: Rename all log* methods

Review commit: https://reviewboard.mozilla.org/r/241686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241686/

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

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

try is green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1987fbb1ad6b9658d85f08bacec660e06a1e75d&group_state=expanded

Comment 7

[Julian Descottes [:jdescottes]]

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

Comment 8

[Julian Descottes [:jdescottes]]

(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.

Comment 9

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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 10

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241686/diff/1-2/

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241686/diff/2-3/

Comment 12

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241686/diff/3-4/

Comment 13

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8973193 [details]
Bug 1458203 - telemetry.js: Rename all log* methods

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241686/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d555b3a047d1
telemetry.js: Rename all log* methods r=jdescottes

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d555b3a047d1