1886702 - use_counter_metrics.yaml takes more than a second to load

Status: NEW

(Toolkit :: Telemetry, defect, P4)

Description

[[:sergesanspaille]]

In [11]: %timeit json.load(open("/home/ssp/sources/mozilla-unified/dom/base/use_counter_metrics.json"))
6.54 ms ± 32.2 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)

In [12]: %timeit yaml.safe_load(open("/home/ssp/sources/mozilla-unified/dom/base/use_counter_metrics.yaml"))
1.72 s ± 169 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [13]: %timeit yaml.load(open("/home/ssp/sources/mozilla-unified/dom/base/use_counter_metrics.yaml"), Loader=yaml.CSafeLoader)
211 ms ± 823 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

where use_counter_metrics.json is an automatic translation of use_counter_metrics.yaml

Comment 1

[[:sergesanspaille]]

Attachment: Bug 1886702 - Generate a json version of use_counter_metrics.yaml r=emilio!

The json version loads at the order of millisecond while the yaml
version loads within a second.

Comment 2

[Alessio Placitelli [:Dexter]]

Hey,

why is this change needed given bug 1886479 ?

Note that this change might have impact on the ingestion and will need to be reviewed by a telemetry peer.

The best component for this is perhaps Toolkit::Telemetry.

Comment 3

[[:sergesanspaille]]

This blocks bug 1886479 because a quater of the time spent in glean_parser is spent parsing this file.

Comment 4

[Alessio Placitelli [:Dexter]]

(In reply to [:sergesanspaille] from comment #3)

This blocks bug 1886479 because a quater of the time spent in glean_parser is spent parsing this file.

Why aren't the changes in bug 1886479 not sufficient? Why do we need to read the YAML file and then produce a JSON?

Comment 5

[Jan-Erik Rediger [:janerik]]

As Alessio said we definitely need to double-check that it works in our pipeline if we get the JSON file (for our use case JSON is a subset of YAML and thus our parsers should handle it just fine, but we still need to make sure that we don't expect a .yaml file suffix somewhere).

For why the patch is the way it is I understand:

dom/base/use_counter_metrics.yaml is generated using a mach command and checked-in. This change also generates the JSON file.
glean_parser seems to correctly handle JSON files already, so swapping it out in the metrics_index.py, which is used to generate code at build time, continues to work.

Comment 6

[Jan-Erik Rediger [:janerik]]

If we want to land this, there's still a bunch of changes along the way we will have to fix first:

• glean_parser actually switches which parser to use: https://github.com/mozilla/glean_parser/blob/d18f672619f885dcc73d51afc3ccd031c3074027/glean_parser/util.py#L122-L124
• but the wrapper around the yaml parser adds important fields: https://github.com/mozilla/glean_parser/blob/d18f672619f885dcc73d51afc3ccd031c3074027/glean_parser/util.py#L83-L87
• later parts unconditionally expect these fields to exist: https://github.com/mozilla/glean_parser/blob/d18f672619f885dcc73d51afc3ccd031c3074027/glean_parser/parser.py#L238
• probe-scraper currently requires yaml suffixes: https://github.com/mozilla/probe-scraper/blob/8b700c4b84a5559dd858dbc7fb7d012220107dc3/probeinfo_api.yaml

Comment 7

[[:sergesanspaille]]

I've submitted https://github.com/mozilla/glean_parser/pull/679 to make the json parser add the same fields as the yaml parser, although there's no hook in the json parser to grab the lineno, so I just set them to null.
I've also updated the attached review to use the glean?util.yaml_load loader before dumping the resulting object to json, so that we remain consistent with any extra step there.

Comment 8

[Jan-Erik Rediger [:janerik]]

Thanks! I checked in with Serge ot understand the "why" better. The tracking bug 1646939 has some context:
The export task runs before every compile and currently takes ~20s or so. The glean_parser invocation is 1-2s of that, so definitely a significant part. Reducing that would help overall (of course compared to compile it's a little amount of time, but this task runs often!).

We will have the C yaml parser soon, which reduces the time somewhat. The JSON patches are not too hard, but we'll defer them for now due to our current workload.

Comment 9

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:chutten, could you have a look please?

For more information, please visit BugBot documentation.

Comment 10

[Chris H-C :chutten]

Hey, Jan-Erik, do you have an easy way to check how much of a difference the C parser made in the now-in-m-c v13.0.1?

Comment 11

[Jan-Erik Rediger [:janerik]]

:sergesanspaille, got any tracking on how much the c parser now improved things?

Comment 12

[[:sergesanspaille]]

On my laptop: from 5.4s to 2.4s \o/