Closed Bug 1281203 Opened 8 years ago Closed 7 years ago

Link error messages from parse_scalars.py to the docs

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox50 --- affected
firefox55 --- fixed

People

(Reporter: Dexter, Assigned: paavininanda, Mentored)

References

Details

(Whiteboard: [measurement:client][lang=python])

Attachments

(1 file, 5 obsolete files)

When the scalar parsing python script fails due to an error, it would be very useful to directly link to the online documentation.
Blocks: 1276201
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
Priority: P3 → P2
Can you set this up as a mentored bug?
Flags: needinfo?(alessio.placitelli)
Priority: P2 → P3
The Python script parse_scalars.py [1] is responsible for parsing the scalars definition file, validating it and then producing the scalar descriptions.

If some scalar does not comply to the definitions from the documentation [2], an exception is thrown. Here's a DXR search with the exception throwing locations

https://dxr.mozilla.org/mozilla-central/search?q=%22raise+ValueError(%22+path%3Acomponents%2Ftelemetry&redirect=false

We need to add a link to the documentation [2] when such errors are reported.

[1] - https://dxr.mozilla.org/mozilla-central/rev/f97a056ae6235de7855fd8aaa04fb1c8d183bd06/toolkit/components/telemetry/parse_scalars.py
[2] - https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html
Mentor: alessio.placitelli, gfritzsche
Flags: needinfo?(alessio.placitelli)
Whiteboard: [measurement:client] → [measurement:client][lang=python]
Hi, I would like to work on this bug, Could I get more information regarding it?
Flags: needinfo?(alessio.placitelli)
(In reply to theeviltwin from comment #3)
> Hi, I would like to work on this bug, Could I get more information regarding
> it?

Hi! Some directions are listed in comment 2: is there anything you want me to expand on?
Flags: needinfo?(alessio.placitelli) → needinfo?(theeviltwin)
If I am understanding this right. All we need to do is add a link to the documentation when the exception is raised.
If I am wrong please correct me on this? Also will the link be the same for all the exceptions?
Thank You
Flags: needinfo?(theeviltwin) → needinfo?(alessio.placitelli)
(In reply to theeviltwin from comment #5)
> If I am understanding this right. All we need to do is add a link to the
> documentation when the exception is raised.

Yes, that's correct.

> If I am wrong please correct me on this? Also will the link be the same for
> all the exceptions?
> Thank You

You're welcome! That's a good question: if there's a specific section related to the problem in the documentation page (e.g. maximum length), then let's link to that section instead. You can get section permalinks by hovering on the title of the section. Otherwise, let's use the general documentation link mentioned in comment 2.
Flags: needinfo?(alessio.placitelli)
Cool, Will get on it right away.
Assignee: nobody → theeviltwin
Status: NEW → ASSIGNED
Attached patch patch1.patch (obsolete) — Splinter Review
Added the links according to my understanding. Do tell me if there are any issues. Thank You
Flags: needinfo?(alessio.placitelli)
Attachment #8784756 - Flags: review?(alessio.placitelli)
Comment on attachment 8784756 [details] [diff] [review]
patch1.patch

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

This looks like a good foundation, thank you for submitting the patch.
Let's refactor the documentation URL to a variable and use that to format the messages.

I just noticed the previous DXR search that I linked didn't include all the exception we raise in that file. Here's an updated search link: https://dxr.mozilla.org/mozilla-central/search?q=%22raise+%22+file%3Aparse_scalars.py&redirect=false

Could you update the patch with the missing links for the other exceptions? Sorry for the inconvenience.

::: toolkit/components/telemetry/parse_scalars.py
@@ +47,4 @@
>          MAX_NAME_LENGTH = 40
>          for n in [group_name, probe_name]:
>              if len(n) > MAX_NAME_LENGTH:
> +                raise ValueError("Name '{}' exceeds maximum name length of {} characters. Refer to documnetation link: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html#string-type-restrictions "\

Could you please move "https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html" to a variable and use that to format the message?

This, for example, could become:

raise ValueError("Name '{}' exceeds maximum name length of {} characters. Refer to documentation link: {}#string-type-restrictions "\.format(n, MAX_NAME_LENGTH, DOCS_BASE_URL))

Same for the other messages.

One last thing, this should link to the "#the-yaml-definition-file" fragment, not "#string-type-restriction"

@@ +53,5 @@
>          def check_name(name, error_msg_prefix, allowed_char_regexp):
>              # Check if we only have the allowed characters.
>              chars_regxp = r'^[a-zA-Z0-9' + allowed_char_regexp + r']+$'
>              if not re.search(chars_regxp, name):
> +                raise ValueError(error_msg_prefix + " name must be alpha-numeric. Got: '{}'. Refer to documnetation link: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html".format(name))

Typo: documnetation -> documentation (also in the other locations)

This should link to the "#the-yaml-definition-file" fragment

@@ +144,5 @@
>  
>          # Validate the collection policy.
>          collection_policy = definition.get('release_channel_collection', None)
>          if collection_policy and collection_policy not in ['opt-in', 'opt-out']:
> +            raise ValueError(self._name + ' - unknown collection policy: ' + collection_policy + ' Refer documnetation link: https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html#')

I think this should link to the Optional Fields section.
Attachment #8784756 - Flags: review?(alessio.placitelli) → feedback+
Sure, Will make the changes accordingly
Attached patch patch2.patch (obsolete) — Splinter Review
There, tried my best to fix everything. Do tell me if I forgot about something.
Flags: needinfo?(alessio.placitelli)
Attachment #8785293 - Flags: review?(alessio.placitelli)
Attachment #8784756 - Attachment is obsolete: true
Comment on attachment 8785293 [details] [diff] [review]
patch2.patch

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

Thank you for updating the patch. We're on the right path: a few links point to the wrong sections and, in general, lines need to be shorter (you can break them).

In order to check if your patch works or breaks the scalar generation script, you can issue the following command:
./mach build toolkit/components/telemetry

If you don't see any error, we should be ok! To further test your patch, you could temporarily change the Scalars.yaml file (see https://dxr.mozilla.org/mozilla-central/rev/1a5b53a831e5a6c20de1b081c774feb3ff76756c/toolkit/components/telemetry/Scalars.yaml) to trigger the various error messages you changed, and see how they look.

Once you tested the updated patch and it looks good, feel free to flag me for another review round!

::: toolkit/components/telemetry/parse_scalars.py
@@ +8,5 @@
>  
>  # The map of containing the allowed scalar types and their mapping to
>  # nsITelemetry::SCALAR_* type constants.
> +
> +global DOC_BASE_URL = 'https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/scalars.html'

Please drop |global| and change the variable name to |BASE_DOC_URL|.

@@ +50,4 @@
>          MAX_NAME_LENGTH = 40
>          for n in [group_name, probe_name]:
>              if len(n) > MAX_NAME_LENGTH:
> +                raise ValueError("Name '{}' exceeds maximum name length of {} characters. Refer documentation link:{}#the-yaml-definition-file"\

Could you change "Refer documentation link:" to "See: "?

Please note that we should not have very long lines of code: the length of a line, should ideally be around 80 characters, as per our style guide (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Line_Length).

Please break the lines accordingly.

@@ +109,5 @@
>  
>          # Do we have any unknown field?
>          unknown_fields = [f for f in definition.keys() if f not in ALL_FIELDS]
>          if len(unknown_fields) > 0:
> +            raise KeyError(self._name + ' - unknown fields: ' + ', '.join(unknown_fields) + '. Refer documentation link:{}#the-yaml-definition-file'.format(DOC_BASE_URL))

This should point to the required-fields section.

@@ +115,5 @@
>          # Checks the type for all the fields.
>          wrong_type_names = ['{} must be {}'.format(f, ALL_FIELDS[f].__name__) \
>              for f in definition.keys() if not isinstance(definition[f], ALL_FIELDS[f])]
>          if len(wrong_type_names) > 0:
> +            raise TypeError(self._name + ' - ' + ', '.join(wrong_type_names) + '. Refer documentation link: {}#the-yaml-definition-file'.format(DOC_BASE_URL))

This should point to the required-fields section.

@@ +124,5 @@
>          for field in list_fields:
>              # Check for empty lists.
>              if len(definition[field]) == 0:
> +                raise TypeError("Field '{}' for probe '{}' must not be empty. Refer documentation link: {}#the-yaml-definition-file"
> +                                .format(field, self._name, DOC_BASE_URL))

This should point to the required-fields section.

@@ +129,5 @@
>              # Check the type of the list content.
>              broken_types =\
>                  [not isinstance(v, LIST_FIELDS_CONTENT[field]) for v in definition[field]]
>              if any(broken_types):
> +                raise TypeError("Field '{}' for probe '{}' must only contain values of type {}. Refer documentation link: {}#the-yaml-definition-file"

This should point to the required-fields section.

@@ +147,5 @@
>  
>          # Validate the collection policy.
>          collection_policy = definition.get('release_channel_collection', None)
>          if collection_policy and collection_policy not in ['opt-in', 'opt-out']:
> +            raise ValueError(self._name + ' - unknown collection policy: ' + collection_policy + '. Refer documentation link: {}#the-yaml-definition-file'.format(DOC_BASE_URL))

This should point to the Optional Fields section.

@@ +235,4 @@
>          with open(filename, 'r') as f:
>              scalars = yaml.safe_load(f)
>      except IOError, e:
> +        raise Exception('Error opening ' + filename + ': ' + e.message + '. Refer documentation link: {}'.format(DOC_BASE_URL))

Leave this line unchanged, it doesn't need to refer to the online docs.
Attachment #8785293 - Flags: review?(alessio.placitelli)
Pranaydeep Singh, did you get stuck somewhere and need help? If so, feel free to ask either here or over IRC (#introduction)!
Flags: needinfo?(theeviltwin)
Clearing the ownership of the bug due to inactivity, so that people can work on it if they want. :theeviltwin, feel free to take it back if you still want to work on it.
Assignee: theeviltwin → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(theeviltwin)
Mentor: alessio.placitelli
I would like to work on this bug!
Flags: needinfo?(alessio.placitelli)
Attached patch bug1281203.patch (obsolete) — Splinter Review
Attachment #8847667 - Flags: review?(gfritzsche)
Assignee: nobody → paavininanda
Flags: needinfo?(alessio.placitelli)
Attachment #8785293 - Attachment is obsolete: true
Comment on attachment 8847667 [details] [diff] [review]
bug1281203.patch

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

Sorry for the delay - this looks close, but needs some work and testing to make sure the message are correct.

::: toolkit/components/telemetry/parse_scalars.py
@@ +9,5 @@
>  # The map of containing the allowed scalar types and their mapping to
>  # nsITelemetry::SCALAR_* type constants.
> +
> +BASE_DOC_URL = 'https://gecko.readthedocs.io/en/latest/toolkit/components/\
> +                telemetry/telemetry/collection/scalars.html'

This way will not work correctly, you could test this by
1) Introducing intentional errors to Scalars.yaml or
2) trying it out in interactive python:
"foo\
   bar"
... ends up print:
"foo   bar"

We could use multi-line strings or break them up as:
"foo" +\
"bar"

The same goes for the other changes below.
Attachment #8847667 - Flags: review?(gfritzsche)
Attached patch bug1281203.patch (obsolete) — Splinter Review
Tried making the changes after testing in interactive python. Let me know if some changes still need to be made.
Attachment #8847667 - Attachment is obsolete: true
Attachment #8848718 - Flags: review?(gfritzsche)
Comment on attachment 8848718 [details] [diff] [review]
bug1281203.patch

I'm a little short on time today, moving this review to Alessio.
Attachment #8848718 - Flags: review?(gfritzsche) → review?(alessio.placitelli)
Comment on attachment 8848718 [details] [diff] [review]
bug1281203.patch

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

This looks like a good start, thank you!

It looks like you're mixing tabs and whitespaces for the indentation. For python code, we use 4 spaces indentation (see https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Indentation).

Please also make sure that your patch doesn't produce any linting error. You can check that by running:

> ./mach lint -l flake8 toolkit/components/telemetry/

Make sure to fix these errors before submitting the updated patch.

::: toolkit/components/telemetry/parse_scalars.py
@@ +60,5 @@
>              chars_regxp = r'^[a-zA-Z0-9' + allowed_char_regexp + r']+$'
>              if not re.search(chars_regxp, name):
> +                raise ValueError(error_msg_prefix + " name must be alpha-numeric. Got: '{}'. " \
> +                				 .format(name) + "See: {}#the-yaml-definition-file".format(BASE_DOC_URL))
> +                				 

Please remove the trailing whitespaces.
Attachment #8848718 - Flags: review?(alessio.placitelli)
Attached patch bug1281203.patch (obsolete) — Splinter Review
Tried resolving all the linting errors.
Attachment #8848718 - Attachment is obsolete: true
Attachment #8849595 - Flags: review?(alessio.placitelli)
Comment on attachment 8849595 [details] [diff] [review]
bug1281203.patch

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

This looks better, it still seems to be based on some other patch or an older revision. Could you please fix that?

Also, some lines go beyond the 99 characters per line. Could you please break them?

In general, I saw you did this:

> raise ValueError("Name '{}' exceeds maximum name length of {} characters. See: "
>                  .format(n, MAX_NAME_LENGTH)+"{}#the-yaml-definition-file".format(BASE_DOC_URL))

What you really want to do is something like

> raise ValueError("Name '{}' exceeds maximum name length of {} characters. See: {}#the-yaml-definition-file"
>                  .format(n, MAX_NAME_LENGTH, BASE_DOC_URL))

Just one string and one format.

::: toolkit/components/telemetry/parse_scalars.py
@@ +52,5 @@
>          MAX_NAME_LENGTH = 40
>          for n in [group_name, probe_name]:
>              if len(n) > MAX_NAME_LENGTH:
> +                raise ValueError("Name '{}' exceeds maximum name length of {} characters. See: "
> +                                 .format(n, MAX_NAME_LENGTH)+"{}#the-yaml-definition-file".format(BASE_DOC_URL))

This line is probably too long (> 99 chars)
Attachment #8849595 - Flags: review?(alessio.placitelli)
Attached patch bug1281203.patchSplinter Review
Made the requested changes and pulled the changes from the remote host before creating the patch.
Attachment #8849595 - Attachment is obsolete: true
Attachment #8851657 - Flags: review?(alessio.placitelli)
Comment on attachment 8851657 [details] [diff] [review]
bug1281203.patch

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

Unfortunately, this doesn't apply cleanly to the tree as the code changed since your last patch. Would you mind rebasing it?

As suggested in the previous comments, please make sure that linting doesn't report any error before submitting your patch:

> ./mach lint -l flake8 toolkit/components/telemetry/

Consider linting as your friend: it will help you catch any style problem before you even submit the patch ;-)

::: toolkit/components/telemetry/parse_scalars.py
@@ +52,4 @@
>          MAX_NAME_LENGTH = 40
>          for n in [group_name, probe_name]:
>              if len(n) > MAX_NAME_LENGTH:
> +                raise ValueError(("Name '{}' exceeds maximum name length of {} characters."

There's no need to wrap the string between a new pair of parentheses here, let's drop the ( and ) you just added. You can add a plus at the end of the string, so that the first line becomes

raise ValueError(("Name '{}' exceeds maximum name length of {} characters." +

@@ +59,5 @@
>          def check_name(name, error_msg_prefix, allowed_char_regexp):
>              # Check if we only have the allowed characters.
>              chars_regxp = r'^[a-zA-Z0-9' + allowed_char_regexp + r']+$'
>              if not re.search(chars_regxp, name):
> +                raise ValueError((error_msg_prefix + " name must be alpha-numeric. Got: '{}'. "

Same here.

@@ +65,4 @@
>  
>              # Don't allow leading/trailing digits, '.' or '_'.
>              if re.search(r'(^[\d\._])|([\d\._])$', name):
> +                raise ValueError((error_msg_prefix + " name must not have a leading/trailing "

Same here.

@@ +134,4 @@
>          for field in list_fields:
>              # Check for empty lists.
>              if len(definition[field]) == 0:
> +                raise TypeError(("Field '{}' for probe '{}' must not be empty" +

Same here.

@@ +140,5 @@
>              # Check the type of the list content.
>              broken_types =\
>                  [not isinstance(v, LIST_FIELDS_CONTENT[field]) for v in definition[field]]
>              if any(broken_types):
> +                raise TypeError(("Field '{}' for probe '{}' must only contain values of type {}"

Same here.
Attachment #8851657 - Flags: review?(alessio.placitelli) → feedback+
I did check for linting errors.The statement above gave "✖ 0 problems (0 errors, 0 warnings)".
Flags: needinfo?(alessio.placitelli)
(In reply to Paavini Nanda from comment #25)
> I did check for linting errors.The statement above gave "✖ 0 problems (0
> errors, 0 warnings)".

Did you try after the changes I requested and after rebasing the patch?
Flags: needinfo?(alessio.placitelli)
Comment on attachment 8851657 [details] [diff] [review]
bug1281203.patch

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

As discussed over IRC, we need the parentheses due to the line length limit, so let's keep them. With that settled, it looks good ;)
Attachment #8851657 - Flags: feedback+ → review+
https://hg.mozilla.org/mozilla-central/rev/eb9816877402
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.