Closed
Bug 1364427
Opened 7 years ago
Closed 7 years ago
Update Histogram probe parsers to probe-scraper test fallout
Categories
(Toolkit :: Telemetry, enhancement, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: gfritzsche, Assigned: gfritzsche)
Details
Attachments
(3 files, 3 obsolete files)
4.27 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
5.08 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
1.67 KB,
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
This comes out of the test runs from here: https://github.com/georgf/probe-scraper/commits/fallout-fix
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8867154 -
Flags: review?(alessio.placitelli)
Assignee | ||
Updated•7 years ago
|
Summary: Make scalars optionally skip strict type checks → Update Histogram probe parsers to probe-scraper test fallout
Assignee | ||
Updated•7 years ago
|
Points: 1 → 2
Updated•7 years ago
|
Attachment #8867154 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8867159 -
Flags: review?(alessio.placitelli)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8867160 -
Flags: review?(alessio.placitelli)
Comment 4•7 years ago
|
||
Comment on attachment 8867159 [details] [diff] [review] Handle old non-numeric expressions in Histograms.json Review of attachment 8867159 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/histogram_tools.py @@ +400,5 @@ > # historical data. > coerce_fields = ["low", "high", "n_values", "n_buckets"] > if not self._strict_type_checks: > + # This handles some old non-numeric expressions. > + expressions = { nit: since this is constant, can we make it upper case -> EXPRESSIONS?
Attachment #8867159 -
Flags: review?(alessio.placitelli) → review+
Comment 5•7 years ago
|
||
Comment on attachment 8867160 [details] [diff] [review] Allow duplicate keys in Histograms.json in non-strict mode Review of attachment 8867160 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/histogram_tools.py @@ +404,5 @@ > expressions = { > "JS::gcreason::NUM_TELEMETRY_REASONS": 101, > "mozilla::StartupTimeline::MAX_EVENT_ID": 12, > } > + nit: should this be in the other patch? @@ +540,5 @@ > with open(filename, 'r') as f: > try: > + def hook(ps): > + return load_histograms_into_dict(ps, strict_type_checks) > + histograms = json.load(f, object_pairs_hook=hook) You could also use lambdas to define the hook. It doesn't really win us much, so just do as you like :) @@ +546,5 @@ > raise ParserError("error parsing histograms in %s: %s" % (filename, e.message)) > return histograms > > > +def from_UseCounters_conf(filename, strict_type_checks): Interesting, I thought that flake8 would complain about strict_type_checks not being used here. Nice that it doesn't!
Attachment #8867160 -
Flags: review?(alessio.placitelli) → review+
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to Alessio Placitelli [:Dexter] from comment #5) > @@ +540,5 @@ > > with open(filename, 'r') as f: > > try: > > + def hook(ps): > > + return load_histograms_into_dict(ps, strict_type_checks) > > + histograms = json.load(f, object_pairs_hook=hook) > > You could also use lambdas to define the hook. It doesn't really win us > much, so just do as you like :) That either makes lines too long and hard to read... or Python lint complain when making that a local var.
Assignee | ||
Comment 7•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8867160 -
Attachment is obsolete: true
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8867154 -
Attachment is obsolete: true
Assignee | ||
Comment 9•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8867159 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8867215 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8867216 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8867220 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/70f16415d8fa Allow duplicate keys in Histograms.json in non-strict mode. r=dexter https://hg.mozilla.org/integration/mozilla-inbound/rev/50d2ae8da001 Make scalars optionally skip strict type checks. r=dexter https://hg.mozilla.org/integration/mozilla-inbound/rev/44db9beed9f4 Handle old non-numeric expressions in Histograms.json. r=dexter
Keywords: checkin-needed
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/70f16415d8fa https://hg.mozilla.org/mozilla-central/rev/50d2ae8da001 https://hg.mozilla.org/mozilla-central/rev/44db9beed9f4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•