1364427 - Update Histogram probe parsers to probe-scraper test fallout

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

This comes out of the test runs from here:
https://github.com/georgf/probe-scraper/commits/fallout-fix

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: Make scalars optionally skip strict type checks

Comment 2

[Georg Fritzsche [:gfritzsche]]

Attachment: Handle old non-numeric expressions in Histograms.json

Comment 3

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow duplicate keys in Histograms.json in non-strict mode

Comment 4

[Alessio Placitelli [:Dexter]]

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?

Comment 5

[Alessio Placitelli [:Dexter]]

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!

Comment 6

[Georg Fritzsche [:gfritzsche]]

(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.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Allow duplicate keys in Histograms.json in non-strict mode

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Make scalars optionally skip strict type checks

Comment 9

[Georg Fritzsche [:gfritzsche]]

Attachment: Handle old non-numeric expressions in Histograms.json

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70f16415d8fa
https://hg.mozilla.org/mozilla-central/rev/50d2ae8da001
https://hg.mozilla.org/mozilla-central/rev/44db9beed9f4