1422849 - Consider dropping support for full display versions (like 61.0a1) from Histograms.json, {Scalars,Events}.yaml

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P3)

Description

[Chris H-C :chutten]

I can think of 0 cases where people have actually wanted to use something other than just a major version number in expires_in_version.

So maybe we should drop it? At the very least we can drop the documentation of it.

Comment 1

[Georg Fritzsche [:gfritzsche]]

I can only find exactly one usage, which is internal and doesn't need it:
https://dxr.mozilla.org/mozilla-central/rev/22d2831cc1f41e1b3e1ebac9be5a7aff33684843/toolkit/components/telemetry/Histograms.json#7732

Let's kill this, i saw no-one use it either.

Comment 2

[Georg Fritzsche [:gfritzsche]]

Same situation for scalars:
https://dxr.mozilla.org/mozilla-central/rev/22d2831cc1f41e1b3e1ebac9be5a7aff33684843/toolkit/components/telemetry/Scalars.yaml#1396

This will need:
- changing the parsers to make anything that is not just a major version (e.g. "60") forbidden in the version check here:
  https://dxr.mozilla.org/mozilla-central/rev/22d2831cc1f41e1b3e1ebac9be5a7aff33684843/toolkit/components/telemetry
/shared_telemetry_utils.py#121
- try `mach build`
- the build should fail for Scalars.yaml & Histograms.json
- fix the two occurences
- `mach build` should now succeed

Comment 3

[Alessio Placitelli [:Dexter]]

Hey Alejandro, would you be interested in working on this?

Comment 4

[Alejandro Rodriguez Salamanca [:alejandro]]

Yes, I'll work on this!

Comment 5

[Alejandro Rodriguez Salamanca [:alejandro]]

> changing the parsers to make anything that is not just a major version (e.g. "60") forbidden in the version check

A major version is always represented as an integer (e.g. "42", "57", "60", etc) or can it also be represented as "42.0", "57.0", etc?

Comment 6

[Alessio Placitelli [:Dexter]]

(In reply to Alejandro Rodriguez Salamanca from comment #5)
> > changing the parsers to make anything that is not just a major version (e.g. "60") forbidden in the version check
> 
> A major version is always represented as an integer (e.g. "42", "57", "60",
> etc) or can it also be represented as "42.0", "57.0", etc?

Just an integer number to make things simpler.

Comment 7

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 8

[Alejandro Rodriguez Salamanca [:alejandro]]

I have seen that there is a method called add_expiration_postfix in shared_telemetry_utils.py (https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/shared_telemetry_utils.py#136) that appends to the expiration version a suffix such as "a1" or ".0a1". Is this method still useful and needed?

Comment 9

[Chris H-C :chutten]

You are correct, it is not still useful or needed.

Comment 10

[Alejandro Rodriguez Salamanca [:alejandro]]

Should I remove it to fix this bug o it's better to create a new one?

Comment 11

[Chris H-C :chutten]

I think removing will be required to fix this bug properly.

Comment 12

[Alejandro Rodriguez Salamanca [:alejandro]]

Okay, thanks chutten! I'll try to submit a new patch with that change ASAP!

Comment 13

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 14

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #11)
> I think removing will be required to fix this bug properly.

tl;dr - I think the "a1" suffix should stay

Mh, I think that the "a1" fixups are still needed. Say we have a probe that expires in version "60": once Nightly is on 60, its version number would be "60.0a1" (because of reasons). When we try to record, we would then check if "60" (coming from the probe definition, without fixups) is less or equal than "60.0a1" (coming from the build info). The internal version comparators will tell us that "60.0" is greater than "60.0a1", so we would keep recording data from the dev channels even though the probe should be expired for that version.

You can test this yourself by going on a privileged page (e.g. about:telemetry) and running |Services.vc.compare("60.0", "60.0a1")| in a browser console.

Comment 15

[Chris H-C :chutten]

Comment on attachment 8937637 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

The patch looks good if we can resolve the 60.0 vs 60.0a1 problem.

Comment 16

[Alessio Placitelli [:Dexter]]

(In reply to Chris H-C :chutten from comment #15)
> Comment on attachment 8937637 [details] [diff] [review]
> Drop support for full display versions in Histograms.json, Scalar.yaml and
> Events.yaml
> 
> Review of attachment 8937637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The patch looks good if we can resolve the 60.0 vs 60.0a1 problem.

+1 - Let's rollback the suffix stuff.

Comment 17

[Chris H-C :chutten]

Comment on attachment 8937228 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

Looks good.

Comment 18

[Chris H-C :chutten]

Do we want to change the docs as well? {events, scalars, histograms}.rst all mention that "N.0" is automatically converted to "N.0a1"

Comment 19

[Alejandro Rodriguez Salamanca [:alejandro]]

(In reply to Chris H-C :chutten from comment #18)
> Do we want to change the docs as well? {events, scalars, histograms}.rst all
> mention that "N.0" is automatically converted to "N.0a1"

Correct me if I'm wrong, but as far as I understand, we still do that. We didn't remove shared_telemetry_utils#add_expiration_postfix. (Well, we did but Dexter pointed out that the a1 suffix should stay https://bugzilla.mozilla.org/show_bug.cgi?id=1422849#c14).

As an example, we can see in https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/parse_histograms.py#318 that add_expiration_postfix is called returning a new expiration with the format "N.0a1".

This makes me wonder if this fix really works. Now shared_telemetry_utils#validate_expiration_version won't match new expirations. Am I missing something here?

Comment 20

[Chris H-C :chutten]

"N" should still be converted to "N.0a1"... but "N.0" is no longer a valid string. So the docs that say " 'N' and 'N.0' will be converted to 'N.0a1'" can drop the `and 'N.0'` parts.

Comment 21

[Alejandro Rodriguez Salamanca [:alejandro]]

Oh, I see. Then, should I remove this? https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/shared_telemetry_utils.py#170

That regexp will match 'N.0' and add the 'a1' postfix, that is no longer supported.

Comment 22

[Chris H-C :chutten]

Leaving it permissive like that should be fine. The code might be exercised when we use these parsers to parse historical editions of Histograms.json (like the probe scraper does)

Speaking of parsing historical editions, this change could break things upstream.

When an upstream system (like the probe scraper or the aggregator) updates their shared_telemetry_utils.py and then tries to parse an old version of Histograms.json that has a ".0" in it, it will fail. That isn't great.

We need to make sure that parse_*.py still work if they're run against old versions of the files, but only if run upstream. Luckily, all upstream systems that use these scripts set self._strict_type_checks to false, so it should be easy to update the parser files.

Sorry to spring these things on you after all this time. I only just remembered it :S

Comment 23

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 24

[Alejandro Rodriguez Salamanca [:alejandro]]

I have updated the docs and added compatibility with old versions still using the 'N.0a1' format. I have assumed that when self._strict_type_checks is set to false, and the expiring version is not 'N' and not 'default', it has to be 'N.0a1'. In other words, if shared_telemetry_utils#validate_expiration_version returns false because the version is different to 'N', and self._strict_type_checks is true, an exception is raised, otherwise, I assume that the format is correct.

Comment 25

[Chris H-C :chutten]

Comment on attachment 8942415 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

Just a few little fixes in the comments for parse_histograms and parse_scalars, and I think we're there.

Oh, and did you fix parse_events.py as well?

::: toolkit/components/telemetry/parse_histograms.py
@@ +310,5 @@
>              ParserError('New histogram "%s" cannot have "default" %s value.' %
>                          (name, field)).handle_later()
>  
> +        # Historical editions of Histograms.json can have the deprecated
> +        # expiration format 'N.0a1'. Fortunately, those scripts set

The code actually allowed for r'^\d{1,3}(\.\d|\.\da1)?$' (so, it included 42, 42.2, 42.2a1 (even though the latter's a bit odd)).

Comment 26

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 27

[Alejandro Rodriguez Salamanca [:alejandro]]

Comments fixed. Regarding parse_events.py, I wrongly assumed that, as it doesn't contain self._strict_type_checks, there aren't any old file using the 'old' format for the expiration, but I just realized that in the submitted patch I fixed Events.yaml. Do you have any suggestion for this to preserve compatibility with old versions of events files?

Comment 28

[Chris H-C :chutten]

Redirecting ni? to :gfritzsche as he would know best about Events and strict_type_checks.

Comment 29

[Georg Fritzsche [:gfritzsche]]

I think we'll need to introduce the same concept of `strict_type_checks` for parse_events.py.
On the client-side we can just require major versions, but upstream [1] we'll need to deal with old file versions.

1: https://github.com/mozilla/probe-scraper

Comment 30

[Chris H-C :chutten]

Alejandro, I've filed bug 1431112 to track implementing the necessary changes to parse_events.yaml. Would you like to take it on?

Comment 31

[Alejandro Rodriguez Salamanca [:alejandro]]

(In reply to Chris H-C :chutten from comment #30)
> Alejandro, I've filed bug 1431112 to track implementing the necessary
> changes to parse_events.yaml. Would you like to take it on?

Yeah, I take it!

Comment 32

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 33

[Alejandro Rodriguez Salamanca [:alejandro]]

Oops, I submitted a patch adding strict_type_checks to parse_events.py here. Rolling back those changes and pushing them to bug 1431112.

Comment 34

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 35

[Chris H-C :chutten]

Comment on attachment 8943472 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

We'll need a new patch taking advantage of parse_events' new strict_type_checks functionality.

Comment 36

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 37

[Alejandro Rodriguez Salamanca [:alejandro]]

I have added a comment, but the line that adds support to historical versions of Events.yaml was added in Bug 1431112 https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/parse_events.py#210

Comment 38

[Chris H-C :chutten]

Comment on attachment 8944953 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

Looks good! And since this doesn't change the data files to a previously-incompatible format, this should require no coordination with upstream consumers of these files. They can update their parsing scripts at their leisure.

::: toolkit/components/telemetry/parse_scalars.py
@@ +190,5 @@
>                                    '.\nSee: {}'.format(BASE_DOC_URL))
>  
>          # Validate the expiration version.
> +        # Historical versions of Scalars.json may contain expiration versions
> +        # using the deprecated format 'N.Na1'. Those scripts set

Technically it's scripts that parse those historical versions that set strict_type_checks (the data files have no capacity to do that)

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3180f034b0
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml r=chutten

Comment 40

[Eliza Balazs [:ebalazs_]]

Backed out changeset 2b3180f034b0 (bug 1422849) for f8 lint failure in /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_histograms.py:319:14

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=2b3180f034b06f37760811fed7b6ee717815bda7&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&selectedJob=158212589

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158212589&repo=mozilla-inbound&lineNumber=219

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a09e41857cf3c9ce6c444222be725f2ba864c9cc&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success

Comment 41

[Alejandro Rodriguez Salamanca [:alejandro]]

Attachment: Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

Comment 42

[Alejandro Rodriguez Salamanca [:alejandro]]

(In reply to Eliza Balazs [:ebalazs_] from comment #40)
> Backed out changeset 2b3180f034b0 (bug 1422849) for f8 lint failure in
> /builds/worker/checkouts/gecko/toolkit/components/telemetry/parse_histograms.
> py:319:14
> 
> Push with failure:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=2b3180f034b06f37760811fed7b6ee717815bda7&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=runnable&filter-
> resultStatus=success&selectedJob=158212589
> 
> Failure log:
> https://treeherder.mozilla.org/logviewer.html#?job_id=158212589&repo=mozilla-
> inbound&lineNumber=219
> 
> Backout:
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=a09e41857cf3c9ce6c444222be725f2ba864c9cc&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&filter-resultStatus=runnable&filter-
> resultStatus=success

It should be fixed now.

Comment 43

[Chris H-C :chutten]

Comment on attachment 8945271 [details] [diff] [review]
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml

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

You can run the linter locally with `./mach lint`. Did that come back with any errors?

Comment 44

[Alejandro Rodriguez Salamanca [:alejandro]]

Running `./mach lint toolkit/components/telemetry/parse_histograms.py`, that it's the file containing the error according to the log https://treeherder.mozilla.org/logviewer.html#?job_id=158212589&repo=mozilla-inbound&lineNumber=219, returns no errors.

Comment 45

[Chris H-C :chutten]

Spiffy! Let's get this back up to the build servers, then.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4637ab31ae11
Drop support for full display versions in Histograms.json, Scalar.yaml and Events.yaml. r=chutten

Comment 47

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/4637ab31ae11