Closed Bug 1364427 Opened 4 years ago Closed 4 years ago

Update Histogram probe parsers to probe-scraper test fallout

Categories

(Toolkit :: Telemetry, enhancement, P1)

enhancement
Points:
2

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: gfritzsche, Assigned: gfritzsche)

Details

Attachments

(3 files, 3 obsolete files)

This comes out of the test runs from here:
https://github.com/georgf/probe-scraper/commits/fallout-fix
Attachment #8867154 - Flags: review?(alessio.placitelli)
Summary: Make scalars optionally skip strict type checks → Update Histogram probe parsers to probe-scraper test fallout
Points: 1 → 2
Attachment #8867154 - Flags: review?(alessio.placitelli) → review+
Attachment #8867159 - Flags: review?(alessio.placitelli)
Attachment #8867160 - Flags: review?(alessio.placitelli)
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 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+
(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.
Attachment #8867160 - Attachment is obsolete: true
Attachment #8867154 - Attachment is obsolete: true
Attachment #8867159 - Attachment is obsolete: true
Attachment #8867215 - Flags: review+
Attachment #8867216 - Flags: review+
Attachment #8867220 - Flags: review+
Keywords: checkin-needed
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
You need to log in before you can comment on or make changes to this bug.