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

Proposing few more attributes #138

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

erikologic
Copy link

First of all, thanks for this amazing action.
I'm loving this so much!

We have just added a self-hosted runner on AWS spot instances: https://github.com/philips-labs/terraform-aws-github-runner
We are bounded by the number of vCPUs we can get from AWS so we need data to understand the best configuration, e.g. whether to have few larger instances and get unit test jobs done faster optimising for compute, or have a bigger fleet of smaller instances.

This PR is an initial iteration at it.
We can use the queued_ms attribute to understand how many jobs were waiting for a runner to pick up the work.
We can also use the propagated workflow attributes to get more insights in the whys a particular job or step is executing.

If you are interested in it or a part, let me know and I'll write something that is agreeable on the standards of this repo.

@@ -64,7 +64,10 @@ describe("listWorkflowRunArtifacts", () => {
statusText: "OK",
config: {},
});
/* eslint-disable */
// @ts-ignore
Copy link
Author

Choose a reason for hiding this comment

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

Got a type issue that blocks commits without this

@nikordaris
Copy link
Contributor

nikordaris commented Jul 14, 2023

I start looking at this now. If I drop the ball on reviewing this please @me to get my attention.

My initial reaction is that some of the attributes added should already be there. I'm going to have to take a deeper look at the PR and your intentions to understand what I'm missing. I really appreciate you taking the time to contribute and share your use case and needs. We'll figure it out!

@nikordaris
Copy link
Contributor

So it looks like the primary thing missing is the job.created_at. it seems like the rest is either parent attribute propagation or derived attributes? Am I understanding this PR correctly? There is another PR that is proposing propagating parent attributes that I'd like to separate these concerns. Can we just focus on the missing attributes for this PR? I'd also like to avoid derived attributes like the MS between created and started. I think this should be calculated on the vendor. I know honeycomb enables this.

@erikologic
Copy link
Author

There is another PR that is proposing propagating parent attributes that I'd like to separate these concerns. Can we just focus on the missing attributes for this PR?

Works for me 👍

I'd also like to avoid derived attributes like the MS between created and started. I think this should be calculated on the vendor. I know honeycomb enables this.

Sounds like a good approach 👍

So it looks like the primary thing missing is the job.created_at.

Yep, given we agreed on all the rest, the only thing that would be worth doing with this PR would be this.

@erikologic
Copy link
Author

I guess I can close this PR and open a new one with that change only then.

FYI when I try to setup things locally I had to wrestle a bit with ESLint and Typescript.
Might be something weird that I have done, but it would beneficial to have some sorts of instructions, like. a CONTRIBUTING.md .
I'll try avoid adding all those disabling and ignoring in the next PR.

@nikordaris
Copy link
Contributor

If you end up figuring out what tripped you up I'd love help getting a contributing doc started!

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.

2 participants