1426460 - Update parse_events.py to make use of extended validator functionality

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Aditya Bharti [:adbugger]]

The shared_telemetry_utils.py file was edited and the ParserError class was extended in bug 1401612. The validator now outputs multiple errors at a time and all ParserErrors have been separated into immediately fatal and eventually fatal. The parse_histograms.py parser has already been updated, this bug is to update the parse_events.py file.

Comment 1

[Swapnil P (:DarthSwap)]

I would like to work on this bug. I have read bug 1401612 and as far as I understand, in the file parse_events.py, I need to update the calls to class ParseError with function calls of handle_now() and handle_later(). I'm having trouble classifying which errors fail the build immediately and which are the ones that can be handled later. Is there something I'm missing that helps me better differentiate between these?

Comment 2

[Chris H-C :chutten]

When a ParserError is made, consider whether it is because the file is now unparseable, or if it's a failure in the values recorded. Generally, most errors will be of the second type, which is handle_later(). handle_now is for errors that result from IO problems, or problems that would make the rest of the errors less useful.

When in doubt, make it a handle_later(), and we'll go over them in review :)

Comment 3

[Swapnil P (:DarthSwap)]

Attachment: Bug1426460.patch

Bug1426460 - Have updated parse_events.py to make use of the handle_now and handle_later functions. Have assigned handle_now to those errors that arise from missing fields or when the file is unparseable. handle_later is used when there is a mismatch in types or when I was unable to decide and was in doubt. r=chutten.

Comment 4

[Chris H-C :chutten]

Comment on attachment 8944230 [details] [diff] [review]
Bug1426460.patch

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

Looks good! I think there are two cases we can change to handle_later.

How have you been testing this? Have you introduced fake events descriptions into Events.yaml to make sure all the handle_laters show at the end, and the handle_nows fail immediately?

::: toolkit/components/telemetry/parse_events.py
@@ +192,5 @@
>          # Check extra_keys.
>          extra_keys = definition.get('extra_keys', {})
>          if len(extra_keys.keys()) > MAX_EXTRA_KEYS_COUNT:
>              raise ParserError("%s: Number of extra_keys exceeds limit %d." %
> +                              (self.identifier, MAX_EXTRA_KEYS_COUNT)).handle_now()

This can be handle_later, I think.

@@ +202,4 @@
>          # Check expiry.
>          if 'expiry_version' not in definition:
>              raise ParserError("%s: event is missing required field expiry_version"
> +                              % (self.identifier)).handle_now()

This can be handle_later

Comment 5

[Swapnil P (:DarthSwap)]

Attachment: Bug1426460.patch

Bug1426460 - I have made the requested changes to parse_events.py. How exactly do I test these changes though? I opened Events.yaml and know to make changes in the testing section but don't know what to run to see the results of the testing.

Comment 6

[Chris H-C :chutten]

Comment on attachment 8944719 [details] [diff] [review]
Bug1426460.patch

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

Can you change your commit message summary line to explain what you did in the entire patch? Something perhaps like "bug 1426460 - Allow parse_events to report more than one parsing error at once r=chutten"

The patch looks great.

Comment 7

[Chris H-C :chutten]

To test, introduce anywhere in Events.yaml an invalid Event definition. Specifically try to hit as many parser errors as possible. Then `./mach build` will run parse_events.py to try and generate the C++ code for the new Events.yaml and should fail with the parser errors you just modified.

Comment 8

[Swapnil P (:DarthSwap)]

Attachment: Bug1426460.patch

Bug1426460 - Allow parse_events to report more than one parsing error at once r=chutten

Comment 9

[Swapnil P (:DarthSwap)]

(In reply to Chris H-C :chutten from comment #7)
> To test, introduce anywhere in Events.yaml an invalid Event definition.
> Specifically try to hit as many parser errors as possible. Then `./mach
> build` will run parse_events.py to try and generate the C++ code for the new
> Events.yaml and should fail with the parser errors you just modified.

I'm facing a few issues on running ./mach build related to virtualenv etc. I'm currently trying to correct this and finding a way to make it work. Once it works I'll test this and give you an update on if the tests are failing succesfully or not.

Comment 10

[Chris H-C :chutten]

That's doesn't sound like much fun. Please do not hesitate to jump on IRC (https://wiki.mozilla.org/Irc has some instructions). There will likely be someone on the #build or #developers channels who might be able to help you through your problems, especially if you post the error log somewhere like pastebin (https://pastebin.mozilla.org/) so they can take a look.

Comment 11

[Chris H-C :chutten]

Comment on attachment 8945322 [details] [diff] [review]
Bug1426460.patch

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

Looks good.

Once your build issues are figured out, and your tests are failing in appropriate ways, consider running this file through `mach lint` to ensure it adheres to the style guidelines.

Comment 12

[Chris H-C :chutten]

Comment on attachment 8945322 [details] [diff] [review]
Bug1426460.patch

r?adbugger, who added the handle_later functionality, for another pair of eyes.

Comment 13

[Aditya Bharti [:adbugger]]

Comment on attachment 8945322 [details] [diff] [review]
Bug1426460.patch

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

As per the extended functionality, the ParserError class itself decides when to raise an error. You do not raise any ParserErrors yourself as the .handle_* methods do not return an Error object.

In this patch, there are multiple lines like `raise ParserError().handle_later()` and `raise ParserError().handle_now()`. Please remove the 'raise' keyword and change these lines to simply `ParserError().handle_later()` and `ParserError().handle_now()`. Overall, there should be no 'raise's with ParserErrors() in any of the parse_*.py files

::: toolkit/components/telemetry/parse_events.py
@@ +46,4 @@
>          if not isinstance(value, self.instance_type):
>              raise ParserError("%s: Failed type check for %s - expected %s, got %s." %
>                                (identifier, key, nice_type_name(self.instance_type),
> +                               nice_type_name(type(value)))).handle_later()

Simply change to ParserError().handle_later(). Remove 'raise'

@@ +52,5 @@
>  class MultiTypeChecker:
>      """Validate a simple value against a list of possible types"""
>      def __init__(self, *instance_types):
>          if not instance_types:
> +            raise Exception("At least one instance type is required.").handle_now()

Only the ParserError class is extended. Please change this line to its previous version. There is no .handle_now() method for the general Exception class.

@@ +61,4 @@
>              raise ParserError("%s: Failed type check for %s - got %s, expected one of:\n%s" %
>                                (identifier, key,
>                                 nice_type_name(type(value)),
> +                               " or ".join(map(nice_type_name, self.instance_types)))).handle_later()

remove raise

@@ +72,4 @@
>      def check(self, identifier, key, value):
>          if len(value) < 1:
>              raise ParserError("%s: Failed check for %s - list should not be empty." %
> +                              (identifier, key)).handle_now()

remove raise

@@ +77,5 @@
>          for x in value:
>              if not isinstance(x, self.instance_type):
>                  raise ParserError("%s: Failed type check for %s - expected list value type %s, got"
>                                    " %s." % (identifier, key, nice_type_name(self.instance_type),
> +                                            nice_type_name(type(x)))).handle_later()

remove raise

@@ +90,4 @@
>      def check(self, identifier, key, value):
>          if len(value.keys()) < 1:
>              raise ParserError("%s: Failed check for %s - dict should not be empty." %
> +                              (identifier, key)).handle_now()

remove raise

@@ +96,5 @@
>                  raise ParserError("%s: Failed dict type check for %s - expected key type %s, got "
>                                    "%s." %
>                                    (identifier, key,
>                                     nice_type_name(self.keys_instance_type),
> +                                   nice_type_name(type(x)))).handle_later()

remove raise

@@ +103,5 @@
>                  raise ParserError("%s: Failed dict type check for %s - "
>                                    "expected value type %s for key %s, got %s." %
>                                    (identifier, key,
>                                     nice_type_name(self.values_instance_type),
> +                                   k, nice_type_name(type(v)))).handle_later()

just listing out all occurrences of raise ParserError() so that you don't miss any

@@ +128,4 @@
>      # Check that all the required fields are available.
>      missing_fields = [f for f in REQUIRED_FIELDS.keys() if f not in definition]
>      if len(missing_fields) > 0:
> +        raise ParserError(identifier + ': Missing required fields: ' + ', '.join(missing_fields)).handle_now()

Another one. Since this is a handle_now(), the ParserError class itself will handle this error by printing all handle_later() errors and exiting. Remove raise.

@@ +132,5 @@
>  
>      # Is there any unknown field?
>      unknown_fields = [f for f in definition.keys() if f not in ALL_FIELDS]
>      if len(unknown_fields) > 0:
> +        raise ParserError(identifier + ': Unknown fields: ' + ', '.join(unknown_fields)).handle_later()

remove raise

@@ +144,4 @@
>      # Length check.
>      if len(value) < min_length:
>          raise ParserError("%s: Value '%s' for field %s is less than minimum length of %d." %
> +                          (identifier, value, field, min_length)).handle_later()

remove raise

@@ +148,3 @@
>      if max_length and len(value) > max_length:
>          raise ParserError("%s: Value '%s' for field %s is greater than maximum length of %d." %
> +                          (identifier, value, field, max_length)).handle_later()

remove raise

@@ +151,4 @@
>      # Regex check.
>      if regex and not re.match(regex, value):
>          raise ParserError('%s: String value "%s" for %s is not matching pattern "%s".' %
> +                          (identifier, value, field, regex)).handle_later()

remove raise

@@ +180,4 @@
>          allowed_rcc = ["opt-in", "opt-out"]
>          if rcc not in allowed_rcc:
>              raise ParserError("%s: Value for %s should be one of: %s" %
> +                              (self.identifier, rcc_key, ", ".join(allowed_rcc))).handle_later()

remove raise

@@ +186,5 @@
>          record_in_processes = definition.get('record_in_processes')
>          for proc in record_in_processes:
>              if not utils.is_valid_process_name(proc):
>                  raise ParserError(self.identifier + ': Unknown value in record_in_processes: ' +
> +                                  proc).handle_later()

remove raise

@@ +192,5 @@
>          # Check extra_keys.
>          extra_keys = definition.get('extra_keys', {})
>          if len(extra_keys.keys()) > MAX_EXTRA_KEYS_COUNT:
>              raise ParserError("%s: Number of extra_keys exceeds limit %d." %
> +                              (self.identifier, MAX_EXTRA_KEYS_COUNT)).handle_later()

remove raise

@@ +202,4 @@
>          # Check expiry.
>          if 'expiry_version' not in definition:
>              raise ParserError("%s: event is missing required field expiry_version"
> +                              % (self.identifier)).handle_later()

remove raise

@@ +207,5 @@
>          # Finish setup.
>          expiry_version = definition.get('expiry_version', 'never')
>          if not utils.validate_expiration_version(expiry_version):
>              raise ParserError('{}: invalid expiry_version: {}.'
> +                              .format(self.identifier, expiry_version)).handle_now()

remove raise

@@ +290,4 @@
>          with open(filename, 'r') as f:
>              events = yaml.safe_load(f)
>      except IOError, e:
> +        raise ParserError('Error opening ' + filename + ': ' + e.message + ".").handle_now()

remove raise

@@ +294,2 @@
>      except ParserError, e:
> +        raise ParserError('Error parsing events in ' + filename + ': ' + e.message + ".").handle_now()

remove raise

@@ +311,4 @@
>  
>          # Make sure that the category has at least one entry in it.
>          if not category or len(category) == 0:
> +            raise ParserError('Category ' + category_name + ' must contain at least one entry.').handle_now()

remove raise

Comment 14

[Swapnil P (:DarthSwap)]

Attachment: Bug1426460.patch

Bug1426460 - Allow parse_events to report more than one parsing error at once r=chutten.

Comment 15

[Chris H-C :chutten]

Comment on attachment 8946550 [details] [diff] [review]
Bug1426460.patch

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

Redirecting review to :adbugger for the first review pass.

Aditya, please flag me for r? after you've taken a look.

Comment 16

[Aditya Bharti [:adbugger]]

Comment on attachment 8946550 [details] [diff] [review]
Bug1426460.patch

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

Looks good syntax wise. :DarthSwap, you may want to check if it builds successfully, because some ParserErrors span multiple lines and I'm not too sure that python will like the combination of whitespaces.

Redirecting to :chutten for another look and final word on what errors to handle now or later.

Comment 17

[Chris H-C :chutten]

Comment on attachment 8946550 [details] [diff] [review]
Bug1426460.patch

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

From the handle_later/_now angle, this looks good. Does it pass `./mach lint`? Does it print what you expect if you add a faulty events declaration to Events.yaml and then `./mach build`?

Comment 18

[Chris H-C :chutten]

We're very close to getting this in, does it pass lint?

Comment 19

[Chris H-C :chutten]

I tried to apply the patch locally, but it doesn't seem to apply to the version of parse_events that's at the tip of the tree. Could you rebase your work and resubmit it?

Comment 20

[Swapnil P (:DarthSwap)]

I'm sorry for my absence. I'm having a pretty rough semester. Could I please get back to you within a week?

Comment 21

[Chris H-C :chutten]

Hey, no worries!

If I don't hear from you by April 2, I'll assume you're still swamped and I'll try to rebase this and get it landed. Take care of your studies, they're more important :)

Comment 22

[Chris H-C :chutten]

Right, time to rebase and get it landed.

Comment 23

[Chris H-C :chutten]

Attachment: 0001-Bug1426460-Allow-parse_events-to-report-more-than-on.patch

Rebased and linted. Quick rubberstamp Alessio?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f400b5e60c
Allow parse_events to report more than one parsing error at once r=chutten

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e0f400b5e60c