1337022 - Fix the regex in the Telemetry event parser

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter]]

The current regular expression in the Telemetry events doc doesn't match [1] the provided example data (e.g. "ui").

> ^[:alpha:][:alnum:_.]+[:alnum:]$

We should fix the regex and update the schemas that live at [2]

[1] - https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/events.html
[2] - https://github.com/mozilla-services/mozilla-pipeline-schemas

Comment 1

[Georg Fritzsche [:gfritzsche]]

The regex in parse_events.py is too strict here.
We should change it and update the docs.

Comment 2

[Federico Padua [:fedepad]]

(In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #1)
> The regex in parse_events.py is too strict here.
> We should change it and update the docs.

I think I have an idea on how to change minimally the "IDENTIFIER_PATTERN" in parse_events.py to match also on "ui" and update the docs.

But I have no idea about the schemas Alessio is talking....should also this patch supposed to take care of those too?

And who should I put as a reviewer after I push for review?

Thanks!

Comment 3

[Alessio Placitelli [:Dexter]]

(In reply to Federico Padua (fedepad) from comment #2)
> (In reply to Georg Fritzsche [:gfritzsche] [at workweek] from comment #1)
> > The regex in parse_events.py is too strict here.
> > We should change it and update the docs.
> 
> I think I have an idea on how to change minimally the "IDENTIFIER_PATTERN"
> in parse_events.py to match also on "ui" and update the docs.
> 
> But I have no idea about the schemas Alessio is talking....should also this
> patch supposed to take care of those too?

The schemas are linked in comment 0. Once this patch is r+, we can work on a PR to update the schemas there.

You can put Georg or me as reviewers.

Comment 4

[Federico Padua [:fedepad]]

Attachment: Bug 1337022 - Fix the regex in the Telemetry event parser.

This patch fixes the regex defined by IDENTIFIER_PATTERN in
toolkit/components/telemetry/parse_events.py to be less strict and fixes the
relative documentation.
To be precise, before this fix, the regex did not allow the category "ui" (which is described
in the documentation at the webpage
https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/events.html)
to be matched so the documentation was wrong.
The reason for that was that the regex required at least one extra character or
number to be in the string, so for example "uig" would have matched.
With this fix the category "ui" is allowed and matched by the new regex and the documentaion
(https://gecko.readthedocs.io/en/latest/toolkit/components/telemetry/telemetry/collection/events.html#limits)
is updated to reflect the change.

Review commit: https://reviewboard.mozilla.org/r/121400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121400/

Comment 5

[Daria]

Hello! Seems like I figured it out. I propose this new regex: "^[a-z]+[a-z0-9]*[_.]?[a-z0-9]+$"

I've tested it:

import re
mylist = ["ui","u","ui_","_ui","ui8","8u","8","qwert","qw_ty","qw_8","qw.9m","u8u_8g","8u_g"]
for x in mylist:
 match = re.search("^[a-z]+[a-z0-9]*[_.]?[a-z0-9]+$",x)
 if match:
  print x + " is fine"
 else: 
   print x + " is not!"	

And the output is:

ui    is fine
u    is not!
ui_    is not!
_ui    is not!
ui8    is fine
8u    is not!
8      is not!
qwert    is fine
qw_ty    is fine
qw_8    is fine
qw.9m    is fine
u8u_8g    is fine
8u_g    is not!

If the fine outputs are eligible, I'd like to be assigned to this bug and submit a patch.

Comment 6

[Daria]

Hello! I propose this new regex: "^[a-z]+[a-z0-9]*[_.]?[a-z0-9]+$"

I've tested it:

import re
mylist = ["ui","u","ui_","_ui","ui8","8u","8","qwert","qw_ty","qw_8","qw.9m","u8u_8g","8u_g"]
for x in mylist:
 match = re.search("^[a-zA-Z]+[a-zA-Z0-9]*[_.]?[a-zA-Z0-9]+$",x)
 if match:
  print x + " is fine"
 else: 
   print x + " is not!"	

And the output is:

ui    is fine
u    is not!
ui_    is not!
_ui    is not!
ui8    is fine
8u    is not!
8      is not!
qwert    is fine
qw_ty    is fine
qw_8    is fine
qw.9m    is fine
u8u_8g    is fine
8u_g    is not!

If the fine outputs are eligible, I'd like to be assigned to this bug and submit a patch.

Comment 7

[Georg Fritzsche [:gfritzsche]]

Hi Daria!

It looks Federico is already working on this bug. I'll follow up separately with a recommendation for a different bug.

Comment 8

[Alessio Placitelli [:Dexter]]

Sunah, does the suggested regex (reported below) look sane to you?

> ^[:alpha:][:alnum:_.]*[:alnum:]$

The original one fails to validate the example identifiers reported in the docs (e.g. "ui"):

> ^[:alpha:][:alnum:_.]+[:alnum:]$

Comment 9

[bugzilla]

Makes sense to me -- parsing this in plain English for my own understanding: the old regex required an alphabetical character as the first character and an alphanumeric character as the last character, and *required* at least one more character in the middle that's alphanumeric or underscore or period. The new regex allows the absence of a middle character.

One thing to note is that the regex still requires the string to be at least 2 characters (which seems like an acceptable limitation to me, at least.)

Comment 10

[Alessio Placitelli [:Dexter]]

Cheers, thanks Sunah!

Comment 11

[Alessio Placitelli [:Dexter]]

Comment on attachment 8848474 [details]
Bug 1337022 - Fix the regex in the Telemetry event parser.

https://reviewboard.mozilla.org/r/121400/#review125546

Thanks Federico, sorry for the delay!

Comment 12

[Federico Padua [:fedepad]]

Comment on attachment 8848474 [details]
Bug 1337022 - Fix the regex in the Telemetry event parser.

https://reviewboard.mozilla.org/r/121400/#review125546

No problem! Trigger try builds just in case! And let me know if needs rebase...

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c0a377462d47
Fix the regex in the Telemetry event parser. r=Dexter

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Federico Padua (fedepad) from comment #12)
> Comment on attachment 8848474 [details]
> Bug 1337022 - Fix the regex in the Telemetry event parser.
> 
> https://reviewboard.mozilla.org/r/121400/#review125546
> 
> No problem! Trigger try builds just in case! And let me know if needs
> rebase...

Pheew, no need for a rebased patch ;-) There's just one bit of work left to do.

Would you kindly file a PR against [1] to add the event id regex to the main ping schema [2] so that we can properly validate stuff that comes in?

Feel free to flag me as a reviewer there too.

[1] - https://github.com/mozilla-services/mozilla-pipeline-schemas
[2] - https://github.com/mozilla-services/mozilla-pipeline-schemas/blob/master/telemetry/main.schema.json#L841

Comment 15

[Federico Padua [:fedepad]]

Should be ready. 
So I just a need to open a PR on GitHub?
And with what title and commit message? I guess should be related to this but maybe not the same title, etc....

Let me know and I will open it soon!

Comment 16

[Alessio Placitelli [:Dexter]]

Yes, just open a PR on Github.

"Add regex validation to Telemetry events" could work as title/commit message. Feel free to add additional lines to the commit message as you consider appropriate.

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Alessio Placitelli [:Dexter]]

The PR for the schema lives here: https://github.com/mozilla-services/mozilla-pipeline-schemas/pull/34