1894270 - metadata.ping_schedule breaks for unknown pings (and specifically builtin ones)

Status: RESOLVED FIXED

(Data Platform and Tools :: Glean: SDK, defect, P1)

Description

[Jan-Erik Rediger [:janerik]]

The newly introduced ping_schedule feature in glean_parser and the SDK actually cannot be used for builtin pings.
It will fail during parsing with a KeyError: 'baseline' due to the ping not actually being defined in the same run.
Thus the codegen cannot actually set the schedules_pings list.

It's not entirely clear how to solve this just yet.
builtin pings actually do not use codegen. They are in handwritten code, but even if not they would be part of the SDK and not the generated code of applications.

We might need to reconsider how this information is passed.

Options that come to mind:

• Create a map of all ping -> schedules_pings list and pass that in at initialization time.
• Skip unknown pings, but offer a way to get that list. Then put that into some static object which will be asked at the right time. This might be doable as a m-c & builtin pings-only hack.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hi janerik,

Just checking in here... do you think you've more or less decided on an approach for this? Or are you blocked on feedback?

Comment 2

[Jan-Erik Rediger [:janerik]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

Hi janerik,

Just checking in here... do you think you've more or less decided on an approach for this? Or are you blocked on feedback?

Got pulled away for something else yesterday. I'll summarize what I think we should do today and get Travis' input on that.

Comment 3

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1894270 - Schedule pings alongside their parent ping r?chutten!

This is a bit of a hack to make it all work.
On every submit of a ping we call into schedule_pings to also
schedule ride-along pings.

Preferably we would like to have that in glean-core, but for builtin
pings that won't work because they are not code-generated and thus will
never see their schedules_pings array.
So instead we generate a function in m-c that does the mapping and call
that in the ping wrapper used inside m-c.

Comment 4

[Jan-Erik Rediger [:janerik]]

Instead of writing down how I think it could work I wrote the code to make it work how I think it could work.
This has not yet seen testing, so might be subtle broken.

Both Travis (because Glean) and chutten (because FOG hack) might have opinions on this.

Comment 5

[Travis Long [:travis_]]

This works for me, especially since the primary (and currently only ) use-case for this is Desktop.

Comment 6

[Chris H-C :chutten]

Honestly this is kinda how I expected it was going to be implemented, but by exposing on Ping's constructor the optional list of child pings to ride along (doesn't help with submit_ping_by_name, but I think that's debug-only?). This solution gets us where we need to go faster and isn't incompatible with such an SDK-resident future solution, so I'm cool with it for at least now.

Comment 7

[BMO Github Automation]

Attachment: [mozilla/glean_parser] Bug 1894270 - ping_schedule: Gracefully handle missing ping (#705)

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/75cfb4a02983
Schedule pings alongside their parent ping r=chutten,mach-reviewers

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/75cfb4a02983

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Hi janerik,

Thanks for fixing this! So the good news is that with this patch, my patch in bug 1889123 builds properly. \o/

The maybe bad news is that it doesn't actually appear to work... at least from manual testing. Here's what I do:

1. After building the patch and launching, I sign the build into a Mozilla account
2. I restart the build
3. I visit about:glean, and tell it to send the "baseline" ping
4. I visit the Glean Debug Ping Viewer in my main browser, and see the baseline ping come in, but not the fx-accounts ping that my patch defines.

Is this because the about:glean page somehow bypasses the ping scheduling mechanism?

Comment 11

[Jan-Erik Rediger [:janerik]]

Unfortunately yes, this is a side-effect of about:glean. It uses a non-standard API to submit a ping by name and I forgot to actually modify that as well.

And with that said I realize that even though for other custom pings triggered from within Firefox this should now in theory works, it still won't work for the baseline ping, because that's again triggered through a non-standard code path within glean-core.
D'oh!
I think I have an idea how to get that working and I now know how to test that correctly.

Comment 12

[Julian Descottes [:jdescottes]]

FWIW, the test added here test_fog_ride_along_pings permafails today on artifact builds. I imagine this is just a temporary issue and will go away soon, will check again in a few days

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Just to give a reference to such a failure that Julian mentioned:
https://treeherder.mozilla.org/logviewer?job_id=458645013&repo=try

Comment 14

[Jan-Erik Rediger [:janerik]]

Thanks. Looking into this. The followup bug would change this anyway, so I hope to be able to land that.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you Jan-Erik! I just tested again with an artifact build from today and it's still failing.

Comment 16

[Jan-Erik Rediger [:janerik]]

Oh in artifact builds? Hm, I think I might know why.

Comment 17

[Jan-Erik Rediger [:janerik]]

I can confirm that in a local artifact build the test indeed fails. I'm going to send a fix to disable that test for artifact builds for now.