-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
metrics cardinality for ghalistener #3671
metrics cardinality for ghalistener #3671
Conversation
job_workflow_ref also causes horrible high cardinality so I removed it |
The result of this are metrics that function as actual counters:
|
@nikola-jokic , this is another cardinality fix for metrics. |
If possible this would be great if it could be implemented as a command line flag to change what labels are included in the metric. I think that runner_id and runner_name should be removed by default, but I would like to keep job_workflow_ref. This issue demonstrates how we could have a sane default of labels (I'll concede job_workflow_ref to be off) but I'd like to be able to enable it for my use case. Another great use case is reducing the cardinality of the histograms gha_job_execution_duration and gha_job_startup_duration which currently have 48 buckets. It might be good to be able to supply our own buckets until a consensus is reached about fewer, more reasonable, buckets. edit: The old runners included job_workflow_ref and though high'ish cardinaility there are only a finite amount of workflow names, not increasing forever like runner_id. re: #3185 |
Small update, last week I deployed code identical to this (minus removing job_workflow_ref), plus the bucket fixes and it's been working nicely. |
Hey, I think this is a very good addition. I agree, these labels are just causing problems. Thanks for testing it @jameshounshell ! |
@nikola-jokic It would be really helpful in terms of user experience and performance if this could be added to next release ? we run the SelfHosted runner in large scale and the experience is not great because the metrics are missing often due to high memory usage of listener pod and listeners restarting few times day due to cpu and memory pileup, though I have allocated more resources to it. |
That is why I fixed the linting errors myself, so I can be sure this will go in for the next release |
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.
LGTM
@nikola-jokic awesome, thanks! can't wait for next release!! |
Sorry I was bogged down and missed this. I still wanted to keep job_workflow_ref despite it's high cardinality (not infinite). I'll work up a PR to make it an optional label. Maybe we can discuss it's merits on that PR. |
Hey @jameshounshell, We are starting to work on metrics customizations. We are already designing a solution so please wait for that and we would appreciate your feedback! |
Remove runner_id and runner_name from metrics as these are unique for each ephemeral runner cause a new metric for every action.
fixes #3670