-
Notifications
You must be signed in to change notification settings - Fork 10
Add github actions runner collector #15
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
Add github actions runner collector #15
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.
Thanks for the PR! Left a few comments.
src/http.rs
Outdated
pub(crate) const GH_API_RATE_LIMIT_ENDPOINT: &str = "https://github.com/api/rate_limit"; | ||
|
||
pub(crate) fn get(token: &str, url: &str) -> RequestBuilder { | ||
Client::new() |
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.
Ideally the Client
should be shared between invocations, to be able to reuse existing connections to GitHub and avoiding the delay of setting up an HTTPS connection.
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.
I see,, and how do you envision that implementation, a http client instance in each collector's field or you wanna go "bolder" and go for some sort of global http client for both collectors?
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.
I'd have a single client for all the application.
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.
like a global static ? or some ClientWrapper that all collectors request this wrapper to make an http request through channels or something?
OK, i'll try to ping you on zulip for further discussion about it and to know more during the next days. 👍
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.
Reqwest's clients are really nice because they can be used concurrently and can be freely cloned while still maintaining the benefits of reusing connections. There is no need to use channels thankfully.
I think the best approach is to just have each collector require a Client
in its constructor and store it in each struct:
pub async fn new(config: &Config, http: Client) -> Result<Self>;
...and then provide it while registering the collectors:
let http = Client::new();
GitHubRateLimit::new(&p.config, http.clone());
GithubRunners::new(&p.config, http.clone());
// ...
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.
ok, great! thank you for your time on the clarification and code example! it's clear now, I'll do it!
So I can do this change for the GithubRunners
only in this PR and defer the refactor of RateLimit
collector when I'll work in #18 on a future PR or i can do both in this PR.
How do you prefer me to work on it for you to review?
(PS - I just don't want to run the risk of overloading you in this PR with this change like it happened in #13 (comment))
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.
I think doing the change just for GithubRunners
right now is better, I'm a bit busy these days and the less I have to review in one batch the better :)
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.
ok, I've pushed the change that add the http client to gh runner's collector. 👍
6c2df91
to
c4c2def
Compare
@pietroalbini I think this one is ready for review again. Only concern open is #15 (comment) that we can discuss further in the next week. and work on it in a different PR? if not, just lemme know you wanna see the http client worked on this PR in zulip if that's the case. I'm happy to do it as you see fit. |
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 good! Final round of review :)
Thank you so much, and sorry for taking long between reviews! |
Ok, this is the first part of the split discussed in #13 and will close #11
summary:
Added config var
MONITORBOT_RUST_ORG_TOKEN
that needs to contain the org'srepo
scoped token to be used to query GHA runners statusAdded config var
MONITORBOT_RUNNERS_REPOS
that will contain the org's repositories to track GHA runners. multiple repos allowed as long as they are under the same org's umbrella. ex syntax:rust-lang/rust,rust-lang/cargo,rust-lang/rust-clippy
Added config var
MONITORBOT_GHA_RUNNERS_REFRESH
that's the refresh rate frequency (in seconds) to update the cached data about the runners statuses. default value is currently120
seconds.Also introduced in this PR the
http
module that will contain the http client wrapper for future collectors so we dontDRY
every time we introduce a new collector that need to make http API requests.@pietroalbini I'll ask you to review this PR before I submit the second (and last) part of #13 that will contain the fix for #14 and a small refactor to move most of the http requests made by the
github_rate_limit
collector to this new module in a more (I Hope) generalized and clean way.