1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P4)

Description

[Georg Fritzsche [:gfritzsche]]

We have a set of Histograms that is only ever used in tests:
https://dxr.mozilla.org/mozilla-central/search?q=path%3AHistograms.json+TELEMETRY_TEST_

TelemetrySession gets setup with a |testing| argument, we should store that as |this._testing| and only put the TELEMETRY_TEST_* histograms in the payload if it is set.

Comment 1

[Robert Wood [:rwood]]

I'd like to take this. If I'm understanding correctly, the _testing property would be set in:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1347

and

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1458

And check that flag when assembling payload:

https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetrySession.jsm#1196

Am I on the right track? Thanks!

Comment 2

[Georg Fritzsche [:gfritzsche]]

That sounds right, thanks for checking into this.
More specifically, we can check the histogram names for this in getHistograms() & getKeyedHistograms().

Ideally the tests still run through fine:
mach test toolkit/components/telemetry/tests/unit

If not, i'm happy to help through the fallout here.

Comment 3

[Robert Wood [:rwood]]

Attachment: MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

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

Comment 4

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

https://reviewboard.mozilla.org/r/39555/#review36327

Thanks, this looks good.
I only have some style comments below etc.

::: toolkit/components/telemetry/TelemetrySession.jsm:898
(Diff revision 1)
>      let ret = {};
>  
>      for (let name of registered) {
>        for (let n of [name, "STARTUP_" + name]) {
>          if (n in hls) {
> +          if (n.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {

We should comment on why we are doing this.

::: toolkit/components/telemetry/TelemetrySession.jsm:899
(Diff revision 1)
>  
>      for (let name of registered) {
>        for (let n of [name, "STARTUP_" + name]) {
>          if (n in hls) {
> +          if (n.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {
> +            this._log.trace('not testing, skipping ' + n);

`...trace("getHistograms - Skipping test histogram: " + n)`

::: toolkit/components/telemetry/TelemetrySession.jsm:937
(Diff revision 1)
>      let registered =
>        Telemetry.registeredKeyedHistograms(this.getDatasetType(), []);
>      let ret = {};
>  
>      for (let id of registered) {
> +      if (id.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {

We should add a comment here too.
Here a lot of lines get indented, i would prefer an `if (...skip) { ... continue;` pattern for reduced nesting.

::: toolkit/components/telemetry/TelemetrySession.jsm:938
(Diff revision 1)
>        Telemetry.registeredKeyedHistograms(this.getDatasetType(), []);
>      let ret = {};
>  
>      for (let id of registered) {
> +      if (id.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {
> +        this._log.trace('not testing, skipping ' + id);

`...trace("getKeyedHistograms - Skipping test histogram: " + id)`

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

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

Comment 6

[Robert Wood [:rwood]]

https://reviewboard.mozilla.org/r/39555/#review36327

Thanks for the review, patch updated.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

https://reviewboard.mozilla.org/r/39555/#review36563

Thanks! A few more minor things, then this is good to land.

::: toolkit/components/telemetry/TelemetrySession.jsm:898
(Diff revisions 1 - 2)
>      let ret = {};
>  
>      for (let name of registered) {
>        for (let n of [name, "STARTUP_" + name]) {
>          if (n in hls) {
> +          // omit telemetry test histograms from the payload if testing flag is not set

Nit: Start comment with upper-case 'O', finish with '.'.
"Omit ... outside of tests." would tell us more about the intent here.

::: toolkit/components/telemetry/TelemetrySession.jsm:899
(Diff revisions 1 - 2)
>  
>      for (let name of registered) {
>        for (let n of [name, "STARTUP_" + name]) {
>          if (n in hls) {
> +          // omit telemetry test histograms from the payload if testing flag is not set
>            if (n.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {

Missed it before - let's use `startsWith()` instead of `indexOf()`.

::: toolkit/components/telemetry/TelemetrySession.jsm:938
(Diff revisions 1 - 2)
>      let registered =
>        Telemetry.registeredKeyedHistograms(this.getDatasetType(), []);
>      let ret = {};
>  
>      for (let id of registered) {
> +      // omit telemetry test histograms from the payload if testing flag is not set

Ditto on casing, '.' and "... outside of tests".

::: toolkit/components/telemetry/TelemetrySession.jsm:939
(Diff revisions 1 - 2)
>        Telemetry.registeredKeyedHistograms(this.getDatasetType(), []);
>      let ret = {};
>  
>      for (let id of registered) {
> +      // omit telemetry test histograms from the payload if testing flag is not set
>        if (id.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {

Ditto on `startsWith()`.

Comment 8

[Robert Wood [:rwood]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

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

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

https://reviewboard.mozilla.org/r/39555/#review36623

Thanks!
This looks mostly fine to me with the below changes (no need to re-request review).

::: toolkit/components/telemetry/TelemetrySession.jsm:899
(Diff revisions 2 - 3)
>  
>      for (let name of registered) {
>        for (let n of [name, "STARTUP_" + name]) {
>          if (n in hls) {
> -          // omit telemetry test histograms from the payload if testing flag is not set
> -          if (n.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {
> +          // Omit telemetry test histograms outside of tests.
> +          if (n.startsWith('TELEMETRY_TEST_') == true && this._testing == false) {

No need to compare to `true`, just do `n.startsWith(...) && ...`

::: toolkit/components/telemetry/TelemetrySession.jsm:939
(Diff revisions 2 - 3)
>        Telemetry.registeredKeyedHistograms(this.getDatasetType(), []);
>      let ret = {};
>  
>      for (let id of registered) {
> -      // omit telemetry test histograms from the payload if testing flag is not set
> -      if (id.indexOf('TELEMETRY_TEST_') == 0 && this._testing == false) {
> +      // Omit telemetry test histograms outside of tests.
> +      if (id.startsWith('TELEMETRY_TEST_') == true && this._testing == false) {

Ditto, no need to compare to `true`.

Comment 10

[Robert Wood [:rwood]]

Comment on attachment 8729687 [details]
MozReview Request: Bug 1253633 - Don't put the TELEMETRY_TEST_* histograms into the Telemetry payload outside of tests; r=gfritzsche

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/275232400bbd

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/275232400bbd