CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add metrics for projects using OIDC publishers #14044
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks generally okay, a couple of suggestions and questions inline.
warehouse/oidc/tasks.py
Outdated
metrics = request.find_service(IMetricsService, context=None) | ||
|
||
projects_using_oidc = ( | ||
request.db.query(Project).distinct().join(Project.oidc_publishers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: ‏The query as is is likely to perform sequential scans on the projects
table which is now in the 400k+ range - we may need to add an index. Have you looked into that already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, technically, this is just counting projects that have a publisher configured, but that doesn't mean they're actually using it.
We might want to count FileEvents
from OIDC publishers instead:
select count(*) from file_events where additional->>'publisher_url' is not null;
and if we want to count # of projects, we can do the necessary joins from that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miketheman The compute_2fa_metrics
task appears to be doing the same type of scan across projects, are you suggesting adding an index to the intermediary table between Project and OIDCPublisher or something else? (I haven't looked into anything yet)
@di I was wondering about whether projects that have used OIDC publishers versus configuring them would be nice to track as well. I suspect there will be some dwell time between configuring and actually using for some projects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per @di's suggestion I've distinguished between projects that have configured OIDC and have successfully published a release with OIDC in b9d6c0c. Hopefully the join-chain is what you were expecting?
@miketheman Do we need to change either of these queries to optimize them further? My SQLAlchemy-fu is a bit lacking so any suggestions would be helpful 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to change either of these queries to optimize them further?
I'm gonna say "not yet" for now - let's let the task run and see how it does.
Thanks for the review @miketheman! I took my inspiration from the 2FA metrics task PR, I'll get those comments resolved that you mentioned. |
@sethmlarson Any objections about shipping this Monday instead of today, just in case the queries tax the db too heavily? |
@miketheman No objections here! We should maybe even wait for Wednesday since Monday and Tuesday are a holiday for many folks in the US so I and Ee won't be working those days. |
@miketheman Looks like CI can't proceed without maintainer approval? Also I'm cool to deploy this now, I can create a revert PR in case we need to quickly revert the commit. |
This reverts commit cabce64.
Track the uptake of OIDC publishers on PyPI.