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)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: Dexter, Assigned: paavininanda, Mentored)
References
Details
(Whiteboard: [measurement:client][lang=python])
Attachments
(1 file, 5 obsolete files)
8.13 KB,
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
When the scalar parsing python script fails due to an error, it would be very useful to directly link to the online documentation.
Reporter | ||
Updated•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Priority: P3 → P2
Comment 1•8 years ago
|
||
Can you set this up as a mentored bug?
Flags: needinfo?(alessio.placitelli)
Priority: P2 → P3
Reporter | ||
Comment 2•8 years ago
|
||
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]
Comment 3•8 years ago
|
||
Hi, I would like to work on this bug, Could I get more information regarding it?
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 4•8 years ago
|
||
(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)
Comment 5•8 years ago
|
||
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)
Reporter | ||
Comment 6•8 years ago
|
||
(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)
Comment 7•8 years ago
|
||
Cool, Will get on it right away.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → theeviltwin
Status: NEW → ASSIGNED
Comment 8•8 years ago
|
||
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)
Reporter | ||
Comment 9•8 years ago
|
||
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+
Comment 10•8 years ago
|
||
Sure, Will make the changes accordingly
Comment 11•8 years ago
|
||
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)
Reporter | ||
Updated•8 years ago
|
Attachment #8784756 -
Attachment is obsolete: true
Reporter | ||
Comment 12•8 years ago
|
||
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)
Reporter | ||
Comment 13•8 years ago
|
||
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)
Reporter | ||
Comment 14•8 years ago
|
||
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
Updated•8 years ago
|
Flags: needinfo?(theeviltwin)
Reporter | ||
Updated•7 years ago
|
Mentor: alessio.placitelli
Assignee | ||
Comment 15•7 years ago
|
||
I would like to work on this bug!
Flags: needinfo?(alessio.placitelli)
Assignee | ||
Comment 16•7 years ago
|
||
Attachment #8847667 -
Flags: review?(gfritzsche)
Updated•7 years ago
|
Assignee: nobody → paavininanda
Flags: needinfo?(alessio.placitelli)
Updated•7 years ago
|
Attachment #8785293 -
Attachment is obsolete: true
Comment 17•7 years ago
|
||
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)
Assignee | ||
Comment 18•7 years ago
|
||
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 19•7 years ago
|
||
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)
Reporter | ||
Comment 20•7 years ago
|
||
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)
Assignee | ||
Comment 21•7 years ago
|
||
Tried resolving all the linting errors.
Attachment #8848718 -
Attachment is obsolete: true
Attachment #8849595 -
Flags: review?(alessio.placitelli)
Reporter | ||
Comment 22•7 years ago
|
||
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)
Assignee | ||
Comment 23•7 years ago
|
||
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)
Reporter | ||
Comment 24•7 years ago
|
||
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+
Assignee | ||
Comment 25•7 years ago
|
||
I did check for linting errors.The statement above gave "✖ 0 problems (0 errors, 0 warnings)".
Flags: needinfo?(alessio.placitelli)
Reporter | ||
Comment 26•7 years ago
|
||
(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)
Reporter | ||
Comment 27•7 years ago
|
||
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+
Reporter | ||
Comment 28•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb98168774022a0153cc5b6f7ef1d8ce6caa8b77 Bug 1281203 - Link error messages from parse_scalars.py to the docs. r=dexter
Comment 29•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb9816877402
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•