Add workflow job metrics to Github webhook server #1814

ColinHeathman · 2022-09-19T22:22:46Z

Relates to issue #1551

This PR adds these metrics:

github_workflow_job_queue_duration_seconds
github_workflow_job_run_duration_seconds
github_workflow_job_conclusions_total
github_workflow_jobs_queued_total
github_workflow_jobs_started_total
github_workflow_jobs_completed_total
github_workflow_job_failures_total

ColinHeathman · 2022-09-19T22:29:08Z

I have tested this locally with a basic installation. I am in the process of testing this change in an environment with some live traffic, so the PR will remain a draft until that's completed.

Nitpicks are welcome in the meantime

mumoshu · 2022-09-19T23:17:25Z

cmd/githubwebhookserver/metrics/workflow_job_metrics_event_reader.go

+	case "in_progress":
+
+		// discard events if the previous event wasn't tracked
+		previousEvent, ok := reader.trackedJobs[job_id]


Does this imply that this feature will work only when you have only one replica of the githubwebhookserver, but not for two or more replicas? (It's a standard webhook server so it is expected to be scaled out to 2 or more replicas whenever the operator decides to do so.

Yes, that's what I had in mind here. If that's not an assumption I can make, then there needs to be some way to share state, or github_workflow_job_status should be dropped

@ColinHeathman We may probably introduce an optional dependency to an external distributed kv store to track jobs later. Or GitHub could enhance their API and/or the workflow job event payload so that we can use some information included in the event to measure the queue duration. In the meantime though, we should omit this as this isn't going to work universally

I've started a discussion here to enhance the API. I believe that is the cleanest path forwards.

@mumoshu Is there another pending reason to add a kv store? I thought about it but I didn't think these metrics would justify the addition.

Alternatively, we could change github_workflow_job_status to a counter, rather than a gauge.
That way queue counts could be calculated by Prometheus, with github_workflow_job_status{job_status="queued"}[1m] - github_workflow_job_status{job_status="in_progress"}[1m]

I can't think of a way to make github_workflow_job_queue work without a kv store though

Thanks for starting the discussion! Let's see how it goes.

s there another pending reason to add a kv store?

No- this is the first time I thought we might need to add it.

I thought about it but I didn't think these metrics would justify the addition.

Same- if this isn't a short-term hard requirement, I'd like not to add the kv store as an ARC dependency. Might be too much.

we could change github_workflow_job_status to a counter, rather than a gauge.

Ah, counts might be good as well! Can we name the metric like github_workflow_jobs_total(job_status="...") though? If it's a count, I think that better follows the canonical metric name convention of Prometheus https://prometheus.io/docs/practices/naming/#metric-names

ColinHeathman · 2022-10-04T00:07:15Z

@mumoshu Please take a look at this new implementation.

This version is stateless. I was able to find the information I need for the metrics from the logs for the jobs. This uses the GH client to download those logs and parse them, so there is a bit of a downside there, but I think having this be stateless is much better.

mumoshu · 2022-10-04T00:17:35Z

@ColinHeathman Hey! Thanks a lot for your effort. Sorry to say, but this approach isn't going to scale and that's actually why we moved from the pull-based autoscaling to webhook-based autoscaling.
GitHub API rate limit can be quite insufficient when you're going to run hundreds of jobs per hour. AFAIK it's 1000/hour with PAT, and 15000/hour with GitHub App organizational installation.
You added an API call per every workflow_job event in the last commit. Assuming we might receive 3 workflow job events for queued, in_progress, and completed, this approach might make ARC with PAT not scale more than 333 jobs/hour, which is quite low. We do call some APIs in other places for e.g. runner graceful shutdown. So the actual limit would be lower.

mumoshu · 2022-10-04T00:48:04Z

@ColinHeathman IMHO, making it stateful alone is fine, as long as the state is allowed to get lost and local. That said, I thought we'd be happy with your alternative mentioned in #1814 (comment), which is to make those counters so that you can still count events per status, while you can also aggregate counts at query time using e.g. Prometheus, which makes it possible to approximately count events per status without introducing a global state to ARC. WDYT?

mumoshu · 2022-10-04T00:51:02Z

That said, your awesome fetchAndParseWorkflowJobLogs and the related code might better fit within another controller/command within ARC(name it e.g. actions-metrics-server) or another application dedicated to monitoring workflow jobs from the information provided by GitHub Actions API, not from ARC.

In the dedicated controller/command/applilcation, you might be able to poll those APIs less frequently than 1 per workflow job event, which prevents rate limit issues while providing the metrics you want.

Note though, providing workflow job event status count metrics per #1814 (comment) is orthogonal to the metrics derived from GitHub Actions. The former is for monitoring ARC and the latter is for monitoring GitHub Actions. Both look useful to me. Just that I think we shouldn't mix the two.

ColinHeathman · 2022-10-04T00:57:05Z

@ColinHeathman IMHO, making it stateful alone is fine, as long as the state is allowed to get lost. That said, I thought we'd be happy with your alternative mentioned in #1814 (comment), which is to make those counters so that you can still count events per status, while you can also aggregate counts at query time using e.g. Prometheus, which makes it possible to approximately count events per status without introducing a global state to ARC. WDYT?

@mumoshu I did make the change to counters here (the gauges are gone) that's well and good.

Runtimes can be measured off of the action=completed event, but it's less accurate than the logs. I think losing that accuracy is fine though.

I currently don't see a way to get Queue times without either comparing events or making an API call for the logs. I think maybe that needs to be a separate issue or PR since it's going to be more difficult.

ColinHeathman · 2022-10-04T00:58:01Z

What do you think of the metric names? I want to be sure that they are correct

mumoshu · 2022-10-04T01:03:26Z

cmd/githubwebhookserver/metrics/metrics.go

+	githubWorkflowJobConclusionsTotal = prometheus.NewCounterVec(
+		prometheus.CounterOpts{
+			Name: "github_workflow_job_conclusions_total",
+			Help: "Conclusions for tracked workflow jobs",


Suggested change

Help: "Conclusions for tracked workflow jobs",

Help: "Total count of conclusions for tracked workflow jobs",

Probably make it consistent with other metric names would be nicer

mumoshu · 2022-10-04T01:16:43Z

@ColinHeathman Thanks for your confirmation. The metrics names look great to me!

mumoshu · 2022-10-04T01:46:21Z

Runtimes can be measured off of the action=completed event, but it's less accurate than the logs. I think losing that accuracy is fine though.

I'm fine with that too. We'd better add some comment in code to let us keep remembering that the run_duration metric count is not ging to be the same as workflow_job_completions_total when you have two or more ARC webhook-server pods.

I currently don't see a way to get Queue times without either comparing events or making an API call for the logs. I think maybe that needs to be a separate issue or PR since it's going to be more difficult.

I agree with you! If it's going to be something like what I've explained in #1814 (comment), that looks good to me.

mumoshu · 2022-11-07T10:12:21Z

@ColinHeathman Hey! Is this ready for review? If it's working for your cluster, that's all right- I'll try resolving merge conflicts, test it on my cluster, and add necessary changes if you're busy.

ColinHeathman · 2022-11-10T02:41:29Z

Hey @mumoshu, this does work in my cluster. We at BenchSci are currently using this as it is, since we have GH Enterprise and don't need to worry so much about the API rate limit.

I was planning on moving this to the dedicated controller/command/applilcation as you suggested in #1814 (comment), but I haven't had the time to do the rework. I don't think I'll be able to get to it until next month.

Feel free to make changes :) I can try to help anyway I can

mumoshu · 2022-11-30T15:09:18Z

@ColinHeathman Hey! I finally had some time to work on it, and #2057's what I intended. Would you mind taking a look when you have some time?

mumoshu · 2022-12-07T10:05:09Z

@ColinHeathman Hey! I'm closing this in favor of #2057. Thank you again for doing 99% of the work in this pull request! I've featured you in our release note to make your awesome work more visible ❤️
#2068

mumoshu · 2022-12-07T10:06:12Z

#2057 will be included in the next release of ARC, 0.27.0. Please feel free to give it a shot and report back any issues and ideas if any 🙏

ColinHeathman requested review from mumoshu and toast-gear as code owners September 19, 2022 22:22

ColinHeathman marked this pull request as draft September 19, 2022 22:24

mumoshu reviewed Sep 19, 2022

View reviewed changes

mumoshu mentioned this pull request Sep 28, 2022

Missing Metrics endpoints #1867

Closed

5 tasks

JamesAskew mentioned this pull request Oct 3, 2022

Expose additional runnerDeployment metrics #1705

Closed

Add workflow job metrics to Github webhook server

94d6979

ColinHeathman force-pushed the job-metrics branch 2 times, most recently from c04eaba to 94d6979 Compare October 4, 2022 00:04

ColinHeathman marked this pull request as ready for review October 4, 2022 00:05

mumoshu reviewed Oct 4, 2022

View reviewed changes

Fix handling of workflow_job.Conclusion

face674

mumoshu mentioned this pull request Nov 30, 2022

Expose workflow job metrics via new actions-metrics-server #2057

Merged

mumoshu closed this Dec 7, 2022

mumoshu added this to the v0.27.0 milestone Dec 7, 2022

dhawalseth mentioned this pull request Jan 31, 2023

action metrics server not publishing metrics #2231

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add workflow job metrics to Github webhook server #1814

Add workflow job metrics to Github webhook server #1814

ColinHeathman commented Sep 19, 2022 •

edited

Loading

ColinHeathman commented Sep 19, 2022

mumoshu Sep 19, 2022

ColinHeathman Sep 20, 2022

mumoshu Sep 20, 2022

ColinHeathman Sep 20, 2022

mumoshu Sep 21, 2022 •

edited

Loading

ColinHeathman commented Oct 4, 2022

mumoshu commented Oct 4, 2022 •

edited

Loading

mumoshu commented Oct 4, 2022 •

edited

Loading

mumoshu commented Oct 4, 2022 •

edited

Loading

ColinHeathman commented Oct 4, 2022

ColinHeathman commented Oct 4, 2022

mumoshu Oct 4, 2022

mumoshu commented Oct 4, 2022

mumoshu commented Oct 4, 2022

mumoshu commented Nov 7, 2022 •

edited

Loading

ColinHeathman commented Nov 10, 2022

mumoshu commented Nov 30, 2022

mumoshu commented Dec 7, 2022

mumoshu commented Dec 7, 2022

	Help: "Conclusions for tracked workflow jobs",
	Help: "Total count of conclusions for tracked workflow jobs",

Add workflow job metrics to Github webhook server #1814

Add workflow job metrics to Github webhook server #1814

Conversation

ColinHeathman commented Sep 19, 2022 • edited Loading

ColinHeathman commented Sep 19, 2022

mumoshu Sep 19, 2022

Choose a reason for hiding this comment

ColinHeathman Sep 20, 2022

Choose a reason for hiding this comment

mumoshu Sep 20, 2022

Choose a reason for hiding this comment

ColinHeathman Sep 20, 2022

Choose a reason for hiding this comment

mumoshu Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

ColinHeathman commented Oct 4, 2022

mumoshu commented Oct 4, 2022 • edited Loading

mumoshu commented Oct 4, 2022 • edited Loading

mumoshu commented Oct 4, 2022 • edited Loading

ColinHeathman commented Oct 4, 2022

ColinHeathman commented Oct 4, 2022

mumoshu Oct 4, 2022

Choose a reason for hiding this comment

mumoshu commented Oct 4, 2022

mumoshu commented Oct 4, 2022

mumoshu commented Nov 7, 2022 • edited Loading

ColinHeathman commented Nov 10, 2022

mumoshu commented Nov 30, 2022

mumoshu commented Dec 7, 2022

mumoshu commented Dec 7, 2022

ColinHeathman commented Sep 19, 2022 •

edited

Loading

mumoshu Sep 21, 2022 •

edited

Loading

mumoshu commented Oct 4, 2022 •

edited

Loading

mumoshu commented Oct 4, 2022 •

edited

Loading

mumoshu commented Oct 4, 2022 •

edited

Loading

mumoshu commented Nov 7, 2022 •

edited

Loading