1426461 - Update parse_scalars.py to make use of extended validator functionality

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, 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_scalars.py file.

Comment 1

[Ashish Verma [:ashish1500616]]

Hello I would like to work on this bug.?
Can I get started.

Comment 2

[Chris H-C :chutten]

You certainly may! To start, I recommend reading bug 1401612 paying close attention to the patch that was eventually submitted. Your work will be to do a very similar kind of thing, but for the parse_scalars.py file.

If you have any questions, please do not hesitate to ask!

Comment 3

[Ashish Verma [:ashish1500616]]

(In reply to Chris H-C :chutten from comment #2)
> You certainly may! To start, I recommend reading bug 1401612 paying close
> attention to the patch that was eventually submitted. Your work will be to
> do a very similar kind of thing, but for the parse_scalars.py file.
> 
> If you have any questions, please do not hesitate to ask!

I am new to the organization and I would need your help.

Comment 4

[Ashish Verma [:ashish1500616]]

(In reply to Chris H-C :chutten from comment #2)
> You certainly may! To start, I recommend reading bug 1401612 paying close
> attention to the patch that was eventually submitted. Your work will be to
> do a very similar kind of thing, but for the parse_scalars.py file.
> 
> If you have any questions, please do not hesitate to ask!

I am new to the organization and I would need your help. What needs to be done to fix the issue?

Comment 5

[Ashish Verma [:ashish1500616]]

What is extended validator functionality?How can I go about fixing the issue?

Comment 6

[Chris H-C :chutten]

Sorry about the delay. I work for Mozilla so you'll generally not see much activity from me on weekends :)

To fix this bug you'll be using the new ParserError functionality "handle_later()" for most of the errors that happen in the file toolkit/components/telemetry/parse_scalars.py

Any errors that should fail the build immediately would be "handle_now()" instead.

parse_scalars.py is run during Firefox's build step. These changes will make it so that a single build step will reveal all the errors in the changes you made, instead of having the build fail immediately with only one problem listed.

Does this make sense?

Comment 7

[Ashish Verma [:ashish1500616]]

Thanks :chutten ,Yes now it make some sense to me.I will start working on it .

Comment 8

[Chris H-C :chutten]

Hello! Do you need any help? Are you still working on this?

Comment 9

[Chris H-C :chutten]

Marking unassigned due to inactivity.

Comment 10

[Dhi Aurrahman]

Attachment: Make use of extended validator functionality

Comment 11

[Chris H-C :chutten]

Comment on attachment 8955749 [details] [diff] [review]
Make use of extended validator functionality

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

Thank you for your patch!

In what ways did you test that this was working? The way it is written, I don't see any handle_now or handle_later calls that would report errors. Did you create a malformed scalar definition to ensure that these errors are being reported properly?

Comment 12

[Dhi Aurrahman]

Hi Chris!

It seems I missed those points on checking the _handle_now or handle_later calls_. Could you help me by elaborating it again? Thanks!

Comment 13

[Chris H-C :chutten]

I covered the basics in comment #6, but put another way:

parse_scalars.py is run during the build step.
At this point, it could find some errors in the Scalars.yaml file
Currently, after we find a single error, we stop and print it
We recently added the ability to handle errors later, so we can find most or all of the errors in one build step
To signify which errors we can handle later, we use .handle_later. These errors will print at the very end of the parse.
If the error arose because we can't actually process any further (like, say, the file is missing) we use .handle_now to print all the accumulated errors immediately.

A patch to fix this bug will mark most of these ParserErrors as handle_later.
To test that your patch is correct, you will need to make some malformed Scalar definitions in Scalars.yaml for the errors to find. This will just be local testing, as I'm not sure we have the framework built just yet for you to be able to write automated tests in the tests/python dir. But you can take a look if you're keen to.

Does this help?

Comment 14

[Dhi Aurrahman]

Attachment: Make use of extended validator functionality

Comment 15

[Chris H-C :chutten]

Comment on attachment 8957046 [details] [diff] [review]
Make use of extended validator functionality

Going to assume you meant to put the r? on the new patch instead of the old one :)

Comment 16

[Chris H-C :chutten]

Comment on attachment 8957046 [details] [diff] [review]
Make use of extended validator functionality

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

This looks good! How did you test your change?

Comment 17

[Dhi Aurrahman]

(In reply to Chris H-C :chutten from comment #16)
> Comment on attachment 8957046 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Review of attachment 8957046 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! How did you test your change?

Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was thinking to automate it but was not so sure.

Comment 18

[Dhi Aurrahman]

(In reply to Chris H-C :chutten from comment #15)
> Comment on attachment 8957046 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Going to assume you meant to put the r? on the new patch instead of the old
> one :)

Ah yeah, sorry for that.

Comment 19

[Chris H-C :chutten]

(In reply to Dhi Aurrahman from comment #17)
> (In reply to Chris H-C :chutten from comment #16)
> > Comment on attachment 8957046 [details] [diff] [review]
> > Make use of extended validator functionality
> > 
> > Review of attachment 8957046 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks good! How did you test your change?
> 
> Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was
> thinking to automate it but was not so sure.

We do have one python test, but it might not be within the scope of this "good next bug" to write one for parse_scalars.

However, that would be an excellent next bug to try, if you're interested. You could file the bug yourself and then use the test_histogramtools_non_strict.py test as an example?

Comment 20

[Dhi Aurrahman]

(In reply to Chris H-C :chutten from comment #19)
> (In reply to Dhi Aurrahman from comment #17)
> > (In reply to Chris H-C :chutten from comment #16)
> > > Comment on attachment 8957046 [details] [diff] [review]
> > > Make use of extended validator functionality
> > > 
> > > Review of attachment 8957046 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > This looks good! How did you test your change?
> > 
> > Hi Chris, I manually introduce the error to the Scalars.yml. Yeah, I was
> > thinking to automate it but was not so sure.
> 
> We do have one python test, but it might not be within the scope of this
> "good next bug" to write one for parse_scalars.
> 
> However, that would be an excellent next bug to try, if you're interested.
> You could file the bug yourself and then use the
> test_histogramtools_non_strict.py test as an example?

Interesting, let me take a look. But since I have no previous experience in writing complicated python scripts, it will take a while.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13163586e52b
Make use of extended validator functionality r=chutten

Comment 22

[Landry Breuil (:gaston)]

Should be backed out (or fixed), triggers lint failures, cf https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=865c06c8aae729a258d9f56d4f292421ecdfae57&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Comment 23

[Chris H-C :chutten]

Please back it out, :gaston.

Dhi, you can run `./mach lint` locally to ensure your change adheres to the style guidelines.

Comment 24

[Landry Breuil (:gaston)]

Well i ain't no sherriff, so i think dluca should do this backout and repush bug 1442746 while here...

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for flake8 linting failure:

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c35cbe59f9201ca16b7b0ff1a1ef77a0a26bf0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=167002221&repo=mozilla-inbound
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:62:86 | statement ends with a semicolon (E703)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:69:100 | line too long (107 > 99 characters) (E501)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:75:100 | line too long (108 > 99 characters) (E501)

Please fix the issues and submit an updated patch.

Comment 26

[Dhi Aurrahman]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #25)
> Backed out for flake8 linting failure:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 35c35cbe59f9201ca16b7b0ff1a1ef77a0a26bf0
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=167002221&repo=mozilla-
> inbound
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 62:86 | statement ends with a semicolon (E703)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 69:100 | line too long (107 > 99 characters) (E501)
> TEST-UNEXPECTED-ERROR |
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_scalars.py:
> 75:100 | line too long (108 > 99 characters) (E501)
> 
> Please fix the issues and submit an updated patch.

Will do

Comment 27

[Dhi Aurrahman]

Attachment: Make use of extended validator functionality

Comment 28

[Chris H-C :chutten]

Comment on attachment 8957724 [details] [diff] [review]
Make use of extended validator functionality

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

Did you use ./mach lint to make sure we got them all?

Comment 29

[Dhi Aurrahman]

(In reply to Chris H-C :chutten from comment #28)
> Comment on attachment 8957724 [details] [diff] [review]
> Make use of extended validator functionality
> 
> Review of attachment 8957724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Did you use ./mach lint to make sure we got them all?

Yes Chris, sorry for the mistake. 

✖ 0 problems (0 errors, 0 warnings)

Comment 30

[Chris H-C :chutten]

Excellent, just checking :)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/782ec59a7264
Make use of extended validator functionality r=chutten

Comment 32

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/782ec59a7264