1357001 - Improve Telemetry python script errors

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Georg Fritzsche [:gfritzsche]]

Currently we throw errors from the histogram, scalar & event parsing python scripts when a condition is not met.
This produces hard to read errors.

We should refactor this to show clear error messages while still failing the build.

We may want to merge the probe-scraper updates into m-c [1] before this, to avoid further rebasing pain later.

1: https://github.com/mozilla/probe-scraper/issues/6

Comment 1

[Georg Fritzsche [:gfritzsche]]

Attachment: First cut

Comment 2

[Chris H-C :chutten]

Comment on attachment 8858770 [details] [diff] [review]
First cut

Review of attachment 8858770 [details] [diff] [review]:
-----------------------------------------------------------------

Some notes.

::: toolkit/components/telemetry/Histograms.json
@@ +5,5 @@
>      "description": "has accessibility support been instantiated"
>    },
>    "A11Y_CONSUMERS": {
>      "expires_in_version": "default",
> +    "kind": "count",

Uh... I don't think this is allowed.

::: toolkit/components/telemetry/gen-histogram-data.py
@@ +180,5 @@
>  def main(output, *filenames):
> +    try:
> +        histograms = list(histogram_tools.from_files(filenames))
> +    except histogram_tools.BuildError as ex:
> +        print("\nError processing histograms:\n" + str(ex) + "\n")

Can we be more specific? "Error processes histograms when generating histogram data: "

::: toolkit/components/telemetry/gen-histogram-enum.py
@@ +45,5 @@
>      # Load the histograms.
> +    try:
> +        all_histograms = list(histogram_tools.from_files(filenames))
> +    except histogram_tools.BuildError as ex:
> +        print("\nError processing histograms:\n" + str(ex) + "\n")

Ditto: "Error processing histograms when generating histogram enum: "

::: toolkit/components/telemetry/histogram_tools.py
@@ +453,5 @@
>                  definition['n_buckets'])
>  
>  
>  # This hook function loads the histograms into an OrderedDict.
> +# It will raise a Exception if duplicate keys are found.

It raises a BuildError, specifically.

@@ +471,5 @@
>  def from_Histograms_json(filename):
>      with open(filename, 'r') as f:
>          try:
>              histograms = json.load(f, object_pairs_hook=load_histograms_into_dict)
>          except ValueError, e:

This change is all about removing the ValueErrors, no?

Comment 3

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #2)
> Comment on attachment 8858770 [details] [diff] [review]
> First cut
> 
> Review of attachment 8858770 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some notes.

This was a WIP patch, no need to review yet.

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Unify validation errors in probe parsing scripts

Comment 5

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Make histogram error message style more consistent

Comment 6

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 3 - Make scalar error messages style more consistent

Comment 7

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 4 - Make event error messages style more consistent

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 5 - Fix linting errors

Comment 9

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8859245 [details] [diff] [review]
Part 1 - Unify validation errors in probe parsing scripts

@gps:
Looking at the pattern used in e.g. gen-histogram-data.py, does that look good for you from a build perspective?

The motivation here is to stop throwing exception stacks at developers for errors in Histograms.json, instead giving them only clear error messages.
So i'm changing the Telemetry build scripts to do the following on errors:
- print error messages with N lines and (just `print()` them)
- fail the build (using `sys.exit(1)`)

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8859246 [details] [diff] [review]
Part 2 - Make histogram error message style more consistent

Besides fixing the messages, i also removed the table_dispatch() helper.
It was giving a very unspecific error message, which seems to better fit on the callers side.
Looking through its uses, i still find table_dispatch() rather hard to read and don't see big value in it.
So i decided to just remove it and refactor table handling a bit.

Comment 11

[Chris H-C :chutten]

Comment on attachment 8859245 [details] [diff] [review]
Part 1 - Unify validation errors in probe parsing scripts

Review of attachment 8859245 [details] [diff] [review]:
-----------------------------------------------------------------

Seems straightforward enough.

::: toolkit/components/telemetry/histogram_tools.py
@@ +451,5 @@
>                  definition['n_buckets'])
>  
>  
>  # This hook function loads the histograms into an OrderedDict.
> +# It will raise a Exception if duplicate keys are found.

May as well say ParserError, no?

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> Created attachment 8860394 [details] [diff] [review]
> Part 5 - Fix linting errors

I should roll this into the other patches, but that would mean a lot of re-basing for the later ones.

Comment 13

[Chris H-C :chutten]

Comment on attachment 8859246 [details] [diff] [review]
Part 2 - Make histogram error message style more consistent

Review of attachment 8859246 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think you should remove table_dispatch. It's been running fine since it was introduced.

If you feel it must go, at least split it into its own commit. It has little to do with the error message improvements going on in this patch.

::: toolkit/components/telemetry/gen-histogram-data.py
@@ +129,5 @@
>  
>      for histogram in histograms:
> +        kind = histogram.kind()
> +        if not kind in table:
> +            raise Exception('Unknown kind "%s" for histogram "%s".' % (kind, histogram.name()))

You're switching from ParserError to Exception. Deliberately?

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8859245 [details] [diff] [review]
Part 1 - Unify validation errors in probe parsing scripts

Review of attachment 8859245 [details] [diff] [review]:
-----------------------------------------------------------------

This is fine. With the exception of raising BaseException, you could have done the same thing by adding "except Exception" everywhere you now use "except ParseError." But it's good practice to have a dedicated error type. So how this is implemented definitely isn't wrong.

::: toolkit/components/telemetry/histogram_tools.py
@@ -86,5 @@
>              whitelists = json.load(f)
>              for name, whitelist in whitelists.iteritems():
>                  whitelists[name] = set(whitelist)
>          except ValueError, e:
> -            raise BaseException('error parsing whitelist (%s)' % whitelist_path)

BaseException should never be raised directly. Raise Exception for a generic error instead.

The reason is special exceptions like KeyboardInterrupt, SystemExit, and GeneratorExit inherit from BaseException whereas Exception is in a different tree from these special exceptions. A catch-almost-everything exception handler is written as "except Exception" so it doesn't catch these special exceptions. The bareword "except:" catches BaseException which can lead to e.g. swallowing ^C requests.

Comment 15

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #13)
> I don't think you should remove table_dispatch. It's been running fine since
> it was introduced.
> 
> If you feel it must go, at least split it into its own commit. It has little
> to do with the error message improvements going on in this patch.

My main problem is that we want specific error messages across the code.
Also passing error messages as parameters i don't see the value of this function anymore.

I wonder what this gains us over just putting the three lines at the caller site.
If instead i just put those three lines at the caller site, i don't have to look up anymore what this function does (i regularly do that).

Any better suggestions for the error message and readability?

Comment 16

[Georg Fritzsche [:gfritzsche]]

(In reply to Chris H-C :chutten from comment #13)
> ::: toolkit/components/telemetry/gen-histogram-data.py
> @@ +129,5 @@
> >  
> >      for histogram in histograms:
> > +        kind = histogram.kind()
> > +        if not kind in table:
> > +            raise Exception('Unknown kind "%s" for histogram "%s".' % (kind, histogram.name()))
> 
> You're switching from ParserError to Exception. Deliberately?

Yes - i've been using ParserError for "error message for parsing/validation failures the user needs to see and possibly fix".

Not finding the `kind` here in the table is a programming error that we might want to see a stack for, so i don't think this should be a ParserError.

Comment 17

[Chris H-C :chutten]

Comment on attachment 8859246 [details] [diff] [review]
Part 2 - Make histogram error message style more consistent

Review of attachment 8859246 [details] [diff] [review]:
-----------------------------------------------------------------

Very well.

Comment 18

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 1 - Unify validation errors in probe parsing scripts. r=chutten, f=gps

Comment 19

[Georg Fritzsche [:gfritzsche]]

Attachment: Part 2 - Make histogram error message style more consistent. r=chutten

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/29237e2192a9
Part 1 - Unify validation errors in probe parsing scripts. r=chutten, f=gps
https://hg.mozilla.org/integration/mozilla-inbound/rev/a73ed8eaedf9
Part 2 - Make histogram error message style more consistent. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/c55c6f471cfb
Part 3 - Make scalar error messages style more consistent. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/b439880e27a1
Part 4 - Make event error messages style more consistent. r=chutten
https://hg.mozilla.org/integration/mozilla-inbound/rev/5cf54ffdded1
Part 5 - Fix linting errors. r=chutten

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/29237e2192a9
https://hg.mozilla.org/mozilla-central/rev/a73ed8eaedf9
https://hg.mozilla.org/mozilla-central/rev/c55c6f471cfb
https://hg.mozilla.org/mozilla-central/rev/b439880e27a1
https://hg.mozilla.org/mozilla-central/rev/5cf54ffdded1