Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC 106: Move wpt.fyi metrics generation into the WPT org #106

Merged
merged 5 commits into from
Mar 8, 2022
Merged

Conversation

foolip
Copy link
Member

@foolip foolip commented Feb 18, 2022

@foolip foolip changed the title RFC: Move wpt.fyi metrics generation into WPT org RFC 106: Move wpt.fyi metrics generation into WPT org Feb 18, 2022
@foolip foolip changed the title RFC 106: Move wpt.fyi metrics generation into WPT org RFC 106: Move wpt.fyi metrics generation into the WPT org Feb 18, 2022
@foolip foolip mentioned this pull request Feb 18, 2022
- Compat 2021
- Interop 2022

wpt-results-analysis depends on wpt-results, a highly compressed form of all full wpt.fyi runs ever, currently ~52k stored in a ~385M Git repository. This makes it possible to load thousands (or all) runs into memory at the same time, speeding up metrics generation.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we rename wpt-results, as I think we don't want people thinking it's a canonical data-source? I'm much happier with making everyone treat wpt.fyi's API as the canonical source, and treat wpt-results as an internal implementation detail of result-analysis. Maybe wpt-results-analysis-cache?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wpt-results-analysis-cache sounds good to me.

Ideally wpt.fyi itself would maintain a representation of results that can be efficiently queried, but that's would be a large undertaking.


## Alternatives

We could merge the metrics generation code into the wpt.fyi repository, but the wpt-results would still need to be standalone.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would put us in an odd state whereby we had wpt.fyi generating CSV files, saving them in its own repo, then wpt.fyi pulling those CSV files, right?

In a sense, having wpt.fyi owning this data makes a lot of sense (it is, ultimately, just another projection of data it already owns), but doing it via CSV files in the repo feels a bit odd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to upload the data files to a storage bucket instead. That's complicated because of GCP rules around exporting service account keys, but we can probably make it work some way or another. It's weird that wpt.fyi fetching data from https://raw.githubusercontent.com/...

Copy link
Contributor

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good idea

@foolip
Copy link
Member Author

foolip commented Mar 8, 2022

More than two weeks have passed, so I consider this RFC accepted. When it comes to implementing this, I wanted to name it just "results-analysis", but discovered we already have https://github.com/web-platform-tests/results-analysis as an archived repo. So I'd like to:

An upside of this is that it'll make the transition a bit easier, since both repos will exist, so switching GitHub pages can be done much more easily.

foolip added a commit to web-platform-tests/results-analysis that referenced this pull request Mar 8, 2022
This merges in Ecosystem-Infra/wpt-results-analysis@7db3a55
which is an unrelated history, to preserve the history from that repo.

This implements web-platform-tests/rfcs#106.
foolip added a commit to web-platform-tests/wpt.fyi that referenced this pull request Mar 8, 2022
@foolip
Copy link
Member Author

foolip commented Mar 8, 2022

This is done now:

Because the old repo still exists, as a side effect, the metrics on wpt.fyi will not change until web-platform-tests/wpt.fyi#2799 is merged and deployed.

@foolip foolip closed this Mar 8, 2022
@foolip foolip reopened this Mar 8, 2022
@foolip foolip merged commit 30a464e into master Mar 8, 2022
@foolip foolip deleted the metrics branch March 8, 2022 13:27
KyleJu pushed a commit to web-platform-tests/wpt.fyi that referenced this pull request Mar 9, 2022
@gsnedders
Copy link
Member

More than two weeks have passed, so I consider this RFC accepted. When it comes to implementing this, I wanted to name it just "results-analysis", but discovered we already have https://github.com/web-platform-tests/results-analysis as an archived repo. So I'd like to:

An upside of this is that it'll make the transition a bit easier, since both repos will exist, so switching GitHub pages can be done much more easily.

It feels slightly unfortunate that it didn't just get moved, thereby meaning all the old issues/PRs are in this old archived repo and links in commit messages don't work. The only upside given here is that it mitigates the GitHub Pages breakage from this, which could equally have been mitigated in other ways (e.g., reuploading a mirror to Ecosystem-Infra to maintain the Pages URLs there, at least in the short term).

@foolip
Copy link
Member Author

foolip commented Mar 10, 2022

The only upside given here is that it mitigates the GitHub Pages breakage from this, which could equally have been mitigated in other ways (e.g., reuploading a mirror to Ecosystem-Infra to maintain the Pages URLs there, at least in the short term).

Ah, I didn't consider that option, that probably would have worked, with at most a small amount of downtime. I agree that would have been better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants