Views for metrics v2-v3

[antoinehashbang]

https://phabricator.endlessm.com/T32156

[liZe]

@wjt As discussed earlier, this branch is not ready to be merged, but I’d appreciate a "quick" review to check that everything is OK so far.

What’s missing and/or wrong:

• conflict (fixed),
• tests and lint,
• strange commits, rebase required,
• migration including V2+V3 views is not included,
• union_all in queries requires columns, not tables,
• channel_id needs to replace channel columns, with a solution to get both V2 and V3 channels.

[wjt]

I'll have to take your word for it that this is right :)

[liZe]

Maybe we should override the __getattr__ method of View to get the attributes of the table when they exist, but that’s not necessary now when everything works with only these two attributes.

[wjt]

Please can we have separate commits for:

• move the events (without modification) from __init__.py to events.py
• move the existing views (without modification) from __init__.py to views.py

[wjt]

As discussed elsewhere: it would be ideal to instead have a channel_id which refers to a row in channel_v3. This might work like this (conceptually)

• before updating these views, select distinct image_id, location, dualboot, live, … from metrics_machine, and ensure all such rows are in channel_v3. To my slight surprise there are "only" 2358 distinct such rows. (There are almost a million distinct metrics_machine rows.)
• in this query, join to ChannelV3 on those 4 fields, and add ChannelV3.id.label("channel_id") to the query() list.

[wjt]

Looking at the generated query, this is joining os_version.request_id = launched_existing_equivalent_flatpak.request_id. I think the reason for distinct() below is that there may be multiple os_version events in the same request as any other given request? The other problem is that there will often be no os_version event in the same request as events like launched_existing_equivalent_flatpak which are recorded at basically arbitrary times while the system is in use. (os_version is recorded when the machine boots; and metrics are uploaded every hour; so if launched_existing_equivalent_flatpak is recorded more than an hour after the machine boots, and more than an hour before it next reboots, then there will not be an os_version in the same request.)

Is this join significant in the query plans, particularly for the views that take longer to build? If so, we could just set os_version to NULL, in the knowledge that NULL means "older than eos4". Not ideal but that's already the status quo with v2, it's hard to get the OS version for any given event.

[liZe]

I think the reason for distinct() below is that there may be multiple os_version events in the same request as any other given request?

Exactly.

The other problem is that there will often be no os_version event in the same request as events like launched_existing_equivalent_flatpak which are recorded at basically arbitrary times while the system is in use.

That’s why there’s an outer join, I think that it solves this problem.

Is this join significant in the query plans, particularly for the views that take longer to build?

Unfortunately, I don’t think so.

[wjt]

Why outerjoin? There should always be exactly one Machine per Request, and one Request per LaunchedEquivalentExistingFlatpakV2, so can it not be an inner join?

[liZe]

Why outerjoin? There should always be exactly one Machine per Request, and one Request per LaunchedEquivalentExistingFlatpakV2, so can it not be an inner join?

That’s what I thought, but that’s not the case with production data. There are so many rows that it’s hard to find the data where there’s something wrong. The difference was small, as far as I can remember, so using join would probably be OK too.

[wjt]

I'm not very keen on the "view" suffix here. Similarly the _view_v2 suffix is a bit opaque.

Perhaps…

• updater_failure: real V2 data
• updater_failure_v3: real V3 data
• updater_failure_v2_as_v3: materialized view of V2 data in V3 format
• updater_failure_all: combined view of v2 & v3 data

I'm not all that keen on those names either and have asked on #os for other suggestions :)

(Here I am about to overcomplicate things, probably, but… we could use a separate postgresql schema to namespace these views. All the current views and tables are in public. Maybe a combined schema containing updater_failure (which is the combined view of v2 and v3 data) and a v2_as_v3 schema containing the materialized view? As I type this, I think this would actually be more confusing.)

[wjt]

@dbnicholson thinks that either updater_failure_v2_as_v3 or updater_failure_view_v2 is fine for the materialized view, and updater_failure_all is OK for the combined view. From our Slack:

i think that naming makes sense, although i don't believe A actually has to change. the events did come from the v2 protocol. the fact that their being views in the v3 format is IMO an implementation detail of v3 being the current format. i don't believe you want to keep viewing the v2 events in v2 format

[wjt]

I guess these are just not yet implemented but this branch is missing the 2 × 3 tables to turn old sequence events into new aggregate events.

[liZe]

They’re not created yet, that’s right.

[liZe]

I wish I had some extra time to finally merge this PR…

I’ve marked as "resolved" the comments of @wjt’s review that are now resolved. Here’s what’s missing:

• using channel_id instead of the channel attributes;
• renaming views;
• adding aggregate events;
• (optional) overriding View.__getattr__ to wrap more table attributes;
• and finally, rebasing commits to get a nice history.

Don’t hesitate to ping me in this PR if you need anything!