1354036 - Remove debug.js usage from TelemetrySession.jsm

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Georg Fritzsche [:gfritzsche]]

The use of NS_ASSERT() from debug.js in TelemetrySession.jsm looks rather historic.

We can safely:
- remove the debug.js import
- change the NS_ASSERT() uses into Cu.reportError() calls

Comment 1

[Georg Fritzsche [:gfritzsche]]

Pauline, are you interested in working on this bug?

I think we should just change `NS_ASSERT()` uses [1] into `this._log.error()` calls, not `Cu.reportError()`:
That matches the rest of the module and makes it follow Telemetry log configurations.

After making changes we should:
- `mach build`
- see that tests pass: `mach test toolkit/components/telemetry/tests/unit`
- check that linting passes: `mach eslint`

1: https://dxr.mozilla.org/mozilla-central/search?q=NS_ASSERT(+path%3Atoolkit%2Fcomponents%2Ftelemetry%2F&redirect=false

Comment 2

[Pauline]

Attachment: removed debug.js import; changed `NS_ASSERT()` calls (2) into `this._log.error()` calls

Hi Georg,

Here is my patch. ESLint reported no issues and all tests passed. I noticed you mention there was only 1 use of `NS_ASSERT()`, however I found 2, and I replaced both of these uses. I also removed the debug.js import instructed in the first comment.

Let me know if I need to change anything.

Best,
Pauline

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856130 [details] [diff] [review]
removed debug.js import; changed `NS_ASSERT()` calls (2) into `this._log.error()` calls

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

Thanks, this looks good already!

One smaller thing:
Lets change the commit message to match our standard format [1]: "Bug <NNN> - What changed. r=<reviewer>".
I think the commit message can also be more higher-level, goal-oriented. E.g. "Removed use of NS_ASSERT() from Telemetry."
For smaller details we can view the diff of the changes.

I also commented on a small style improvement below.

1: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1128,5 @@
>          // isn't present, that indicates the distinguished amounts have changed
>          // and this file hasn't been updated appropriately.
>          let amount = mgr[amountName];
> +        if (amount === undefined) {
> +          this._log.error("telemetry accessed an unknown distinguished amount");

While not strictly needed here, lets change the logs to the style of the other `this._log` statements:
"<function name> - Some message."

Comment 4

[Pauline]

Attachment: Removed use of NS_ASSERT() from TelemetrySession.jsm

Hi Georg,

Here is the new patch. I have update the log messages to so that they mention the function in which they occur.
I have also updated my commit message.

Please let me know if anything else is amiss :-)

Best,
Pauline

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856646 [details] [diff] [review]
Removed use of NS_ASSERT() from TelemetrySession.jsm

Side-note:
You can set review or feedback requests (flags) in bugzilla for specific people.
When you "flag" me for review, i get special mail about it and it is listed with other flags in my TODOs.

Comment 6

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856646 [details] [diff] [review]
Removed use of NS_ASSERT() from TelemetrySession.jsm

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

This looks good, thanks!

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8856646 [details] [diff] [review]
Removed use of NS_ASSERT() from TelemetrySession.jsm

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

::: toolkit/components/telemetry/TelemetrySession.jsm
@@ +1129,5 @@
>          // and this file hasn't been updated appropriately.
>          let amount = mgr[amountName];
> +        if (amount === undefined) {
> +          this._log.error("gatherMemory - telemetry accessed an unknown distinguished amount");
> +        }

Actually, i overlooked one thing myself here:

This logging is in a function that is not bound to the instance the surrounding method belongs to.
Logging an error here would throw an exception, because the `this` is a different one than the `this` in the surrounding function.

The easy fix here is to make `h()` an arrow function [1] that automatically binds the surrounding `this`:
> let h = (id, units, amountName) => {
>   ...
> };

I was able to test that relatively easily by:
- temporary make this always log, not just on errors
- ./mach build
- ./mach run
- open tools -> web developer -> browser console [2]
- there, enter:
  - ts = Cu.import("resource://gre/modules/TelemetrySession.jsm"
  - ts.Impl.gatherMemory()
- check that the error log shows up

1: https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Functions/Arrow_functions
2: https://developer.mozilla.org/en/docs/Tools/Browser_Console

Comment 8

[Pauline]

Attachment: Removed use of NS_ASSERT() from TelemetrySession.jsm

Hi, patch updated, hopefully it's all good :-)

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8857061 [details] [diff] [review]
Removed use of NS_ASSERT() from TelemetrySession.jsm

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

Thanks, this looks good now!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/003d697d6686
Removed use of NS_ASSERT() from TelemetrySession.jsm. r=gfritzsche

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/003d697d6686