Update parse_events.py to make use of extended validator functionality

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: adbugger, Assigned: chutten, Mentored)

Tracking

unspecified
mozilla61
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [lang=python][good-next-bug])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Has STR: --- → irrelevant
See Also: → 1401612, 1426461
Mentor: chutten, alessio.placitelli
Status: UNCONFIRMED → NEW
Ever confirmed: true
Priority: -- → P3
Whiteboard: [lang=python][good-next-bug]
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?
Flags: needinfo?(chutten)
(Assignee)

Comment 2

a year ago
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 :)
Flags: needinfo?(chutten)
Posted patch Bug1426460.patch (obsolete) — Splinter Review
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.
Attachment #8944230 - Flags: review?(chutten)
(Assignee)

Comment 4

a year ago
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
Attachment #8944230 - Flags: review?(chutten)
Posted patch Bug1426460.patch (obsolete) — Splinter Review
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.
Attachment #8944230 - Attachment is obsolete: true
Attachment #8944719 - Flags: review?(chutten)
(Assignee)

Comment 6

a year ago
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.
Attachment #8944719 - Flags: review?(chutten) → review+
(Assignee)

Comment 7

a year ago
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.
Posted patch Bug1426460.patch (obsolete) — Splinter Review
Bug1426460 - Allow parse_events to report more than one parsing error at once r=chutten
Attachment #8944719 - Attachment is obsolete: true
Attachment #8945322 - Flags: review?(chutten)
(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.
(Assignee)

Comment 10

a year ago
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.
(Assignee)

Comment 11

a year ago
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.
Attachment #8945322 - Flags: review?(chutten) → review+
(Assignee)

Comment 12

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

r?adbugger, who added the handle_later functionality, for another pair of eyes.
Attachment #8945322 - Flags: review+ → review?(adibhar97)
(Reporter)

Comment 13

a year ago
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
Attachment #8945322 - Flags: review?(adibhar97) → review-
Posted patch Bug1426460.patch (obsolete) — Splinter Review
Bug1426460 - Allow parse_events to report more than one parsing error at once r=chutten.
Attachment #8945322 - Attachment is obsolete: true
Attachment #8946550 - Flags: review?(chutten)
(Assignee)

Comment 15

a year ago
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.
Attachment #8946550 - Flags: review?(chutten) → review?(adibhar97)
(Reporter)

Comment 16

a year ago
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.
Attachment #8946550 - Flags: review?(chutten)
Attachment #8946550 - Flags: review?(adibhar97)
Attachment #8946550 - Flags: review+
(Assignee)

Comment 17

a year ago
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`?
Attachment #8946550 - Flags: review?(chutten) → review+
(Assignee)

Comment 18

a year ago
We're very close to getting this in, does it pass lint?
Assignee: nobody → swapchamps
Status: NEW → ASSIGNED
Flags: needinfo?(swapchamps)
(Assignee)

Comment 19

a year ago
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?
I'm sorry for my absence. I'm having a pretty rough semester. Could I please get back to you within a week?
Flags: needinfo?(swapchamps)
(Assignee)

Comment 21

a year ago
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 :)
(Assignee)

Comment 22

a year ago
Right, time to rebase and get it landed.
Assignee: swapchamps → chutten
Flags: needinfo?(chutten)
(Assignee)

Comment 23

a year ago
Rebased and linted. Quick rubberstamp Alessio?
Attachment #8946550 - Attachment is obsolete: true
Flags: needinfo?(chutten)
Attachment #8971679 - Flags: review?(alessio.placitelli)
Attachment #8971679 - Flags: review?(alessio.placitelli) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 24

a year ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0f400b5e60c
Allow parse_events to report more than one parsing error at once r=chutten
Keywords: checkin-needed

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0f400b5e60c
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.