Closed Bug 1300715 Opened 8 years ago Closed 8 years ago

Forbid adding "expires_in_version":"default" in Histograms.json

Categories

(Toolkit :: Telemetry, defect, P3)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: gfritzsche, Assigned: adamg2, Mentored)

Details

(Whiteboard: [measurement:client] [lang=python][good second bug])

Attachments

(1 file)

Bug 1300715 - Fix issue with indentation
58 bytes, text/x-review-board-request
chutten
: review+
Details 
Historically, when "expires_in_version" was added to Histograms.json [1], "expires_in_version":"default" was the value that existing probes were set to.
New probes however should only use "never" or a specific version (e.g. "53").

Currently it is still possible to add new probes with "default" expiry. We want to forbid that similar to how we handle e.g. "alert_emails" [2]:
* add code supporting the new field to histogram_tools.py
* specifically, allow whitelisting for "default" expiry [3]
* add a whitelist entry called, say, "expiry_default" to histogram-whitelists.json [4]
* add the existing Histograms that use "default" expiry to the whitelist, so the build passes

The build can be tested using "mach toolkit/components/telemetry/".

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Histograms.json
[2] https://dxr.mozilla.org/mozilla-central/search?q=alert_emails+path%3Ahistogram_tools.py&redirect=false
[3] https://dxr.mozilla.org/mozilla-central/rev/dbe4b47941c7b3d6298a0ead5e40dd828096c808/toolkit/components/telemetry/histogram_tools.py#307
[4]
Whiteboard: [measurement:client] [lang=python] → [measurement:client] [lang=python][good second bug]
Assignee: nobody → adamgj.wong
Mentor: chutten
Comment on attachment 8797021 [details]
Bug 1300715 - Fix issue with indentation

https://reviewboard.mozilla.org/r/82638/#review81382

Looks good except for the whitespace.

::: toolkit/components/telemetry/histogram_tools.py:319
(Diff revision 1)
>              if field not in definition and name not in whitelists[field]:
>                  raise KeyError, 'New histogram "%s" must have a %s field.' % (name, field)
>              if field in definition and name in whitelists[field]:
>                  msg = 'Should remove histogram "%s" from the whitelist for "%s" in histogram-whitelists.json'
>                  raise KeyError, msg % (name, field)
> +        

Unrelated whitespace change. Omit
Comment on attachment 8797021 [details]
Bug 1300715 - Fix issue with indentation

https://reviewboard.mozilla.org/r/82638/#review81732

::: toolkit/components/telemetry/histogram_tools.py:319
(Diff revision 2)
>              if field not in definition and name not in whitelists[field]:
>                  raise KeyError, 'New histogram "%s" must have a %s field.' % (name, field)
>              if field in definition and name in whitelists[field]:
>                  msg = 'Should remove histogram "%s" from the whitelist for "%s" in histogram-whitelists.json'
>                  raise KeyError, msg % (name, field)
> -
> +        

Ack, you removed the wrong line. There's end-of-line whitespace here. We don't like end-of-line whitespace (which is why it's highlighted in red.)
Comment on attachment 8797021 [details]
Bug 1300715 - Fix issue with indentation

https://reviewboard.mozilla.org/r/82638/#review82192

LGTM, I'll land it.
Attachment #8797021 - Flags: review?(chutten) → review+
Comment on attachment 8797021 [details]
Bug 1300715 - Fix issue with indentation

https://reviewboard.mozilla.org/r/82636/#review82202

::: toolkit/components/telemetry/histogram-whitelists.json:2094
(Diff revision 3)
> +    "PAGE_FAULTS_HARD",
> +    "BROWSERPROVIDER_XUL_IMPORT_BOOKMARKS",
> +    "PDF_VIEWER_USED",
> +    "NETWORK_DISK_CACHE_OPEN",
> +    "GEOLOCATION_WIN8_SOURCE_IS_MLS"
> -  ]
> +    ]

Drive-by nit: the indentation is off here.
Pushed by chutten@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e353fb70b2a4
prevent newer registered histograms from using default r=chutten
https://hg.mozilla.org/mozilla-central/rev/e353fb70b2a4
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> ::: toolkit/components/telemetry/histogram-whitelists.json:2094
> (Diff revision 3)
> > +    "PAGE_FAULTS_HARD",
> > +    "BROWSERPROVIDER_XUL_IMPORT_BOOKMARKS",
> > +    "PDF_VIEWER_USED",
> > +    "NETWORK_DISK_CACHE_OPEN",
> > +    "GEOLOCATION_WIN8_SOURCE_IS_MLS"
> > -  ]
> > +    ]
> 
> Drive-by nit: the indentation is off here.

It still is.
Flags: needinfo?(chutten)
Your comment came in 17min after I gave it to autoland :S

Adam, could you whip up a patch to fix the indentation?
Flags: needinfo?(chutten) → needinfo?(adamgj.wong)
I'm not sure this patch will apply cleanly. Can you rebase atop the latest mozilla-central (the entire patch should be just the indentation fix)
Was this okay? I get the same rebase error as the other story
Flags: needinfo?(adamgj.wong) → needinfo?(chutten)
For a patch this simple, I'd rewrite it afresh. Just start from an up-to-date mozilla-central tree and fix the whitespace. Then push to mozreview as normal.
Flags: needinfo?(chutten)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: