1938657 - structured missing columns in `firefox_desktop.metrics_v1` for `metrics`.`labeled_counter`.`update.skip_startup_update_reason`.`none`

Status: RESOLVED FIXED

(Data Platform and Tools :: General, defect)

Comment 1

[Ben Wu [:benwu]]

This seems to be coming from bug 1907128. The metric is defined in https://searchfox.org/mozilla-central/source/toolkit/mozapps/update/metrics.yaml#509. This metrics.yaml is used by firefox_desktop_background_update but not firefox_desktop so the column isn't in the normal desktop tables.

:bytesized, is this metric expected to be sent by the firefox desktop glean app as well as the background update task or is this a bug?

Comment 2

[Robin Steuber (she/her) [:bytesized]]

(In reply to Ben Wu [:benwu] from comment #1)

:bytesized, is this metric expected to be sent by the firefox desktop glean app as well as the background update task or is this a bug?

It is expected that it will be sent by both.

Comment 3

[Ben Wu [:benwu]]

In that case, will the values in firefox_desktop be used for analysis? i.e. should it be added as a metric so it shows up as a column in the bigquery table?

I'm not sure if toolkit/mozapps/update/metrics.yaml should be added to firefox_desktop or if a duplicate metric should be defined in another metrics.yaml, since toolkit/mozapps/update/metrics.yaml contains a bunch of unneeded metrics.

Comment 4

[Robin Steuber (she/her) [:bytesized]]

(In reply to Ben Wu [:benwu] from comment #3)

In that case, will the values in firefox_desktop be used for analysis? i.e. should it be added as a metric so it shows up as a column in the bigquery table?

Yes

Comment 5

[Ben Wu [:benwu]]

:janerik, what would be the best way to add this metric to firefox_desktop? toolkit/mozapps/update/metrics.yaml could be reused but I don't know if it matters if unneeded metrics are added

Comment 6

[Jan-Erik Rediger [:janerik]]

We should probably split out that metric and put it into its own file that can be shared with firefox_desktop. toolkit/mozapps/update/shared_metrics.yaml maybe?

Comment 7

[Ben Wu [:benwu]]

bytesized, does moving skip_startup_update_reason to toolkit/mozapps/update/shared_metrics.yaml seem reasonable? Could you do that?

Also looking at the telemetry, move_result, no_window_auto_restarts, suppress_prompts, and version_pin are also being sent in firefox desktop. So if those are expected as well then those can be moved to a shared metrics file too

Comment 8

[Robin Steuber (she/her) [:bytesized]]

I don't really like the naming here. As I understand it, this is the only thing that differentiates this as a "background task" metrics file. Which is to say, nothing in that file or even in-tree seems to indicate that. If we are going to have one file that has desktop and background update pings and one file that has only background update pings, I feel like the latter should be the one that gets the non-standard naming. I'm worried that the proposed naming is going to perpetuate this problem. That is to say, I feel like it's going to be hard to get people to stop putting new desktop/shared probes in metrics.yaml.

Would it be okay if I instead moved the metrics that are only for background updates to their own file? Like background_update_metrics.yaml or something?

Comment 9

[Ben Wu [:benwu]]

Yes that makes sense to me. Feel free to name it whatever is easiest to understand and we'll update repositories.yaml

Comment 10

[Robin Steuber (she/her) [:bytesized]]

Attachment: Bug 1938657 - Split metrics into background task and shared metrics r=benwu!,nalexander!,nrishel!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/576fc9219c6c
Split metrics into background task and shared metrics r=benwu,nalexander,nrishel

Comment 16

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/576fc9219c6c

Comment 17

[Robin Steuber (she/her) [:bytesized]]

Just a heads up, this was done as a result of Bug 1907124, which is now being backed out in Bug 1960981.

I think I am going to backout around this patch rather than backing it out too. I feel like this was a valuable change, even without Bug 1907124.

Hopefully working around this patch won't be too gnarly.