feat(actionsmetrics): Add owner and workflow_name labels to workflow job metrics #2225
Conversation
hamishforbes
requested review from
mumoshu,
toast-gear,
a team and
nikola-jokic
as code owners
pkg/actionsmetrics/metrics.go
Outdated
| ) | ||
| githubWorkflowJobRunDurationSeconds = prometheus.NewHistogramVec( | ||
| prometheus.HistogramOpts{ | ||
| Name: "github_workflow_job_run_duration_seconds", | ||
| Help: "Run times for workflow jobs in seconds", | ||
| Buckets: runtimeBuckets, | ||
| }, | ||
| []string{"runs_on", "job_name", "job_conclusion"}, | ||
| append(commonLabels, "job_conclusion"), |
There was a problem hiding this comment.
I believe this usage of append isn't safe in Go and can produce unexpected results for subsequent append(commonLabels, ...) calls, because append can mutate the slice.
There was a problem hiding this comment.
Ah interesting, sounds like that's more of an issue when using the colon slice notation but yeah seems sketchy!
Would you prefer to just use a literal and duplicate the common labels for each metric or try and retain the re-usable common label definition?
There was a problem hiding this comment.
I do like the idea of explicitly extracting and defining commonLabels!
How about adding a function to create a new copy of commonLabels, add some extra labels, and return the resulting slice? That way, we can repeat the function calls in places we currently refer to commonLabels.
There was a problem hiding this comment.
something lile:
func metricLabels(extras ...string) {
return append(append([[]string{}, commonLabels...), extras...)
}
so that we can rewrite this line to
| append(commonLabels, "job_conclusion"), | |
| metricLabels("job_conclusion"), |
|
I recently merged #2218, and apparently, it contained half of this pull request, resulting in a conflict. Let me try resolving it. |
pkg/actionsmetrics/event_reader.go
Outdated
| @@ -64,6 +64,9 @@ func (reader *EventReader) ProcessWorkflowJobEvent(ctx context.Context, event in | |||
| runsOn := strings.Join(e.WorkflowJob.Labels, `,`) | |||
| labels["runs_on"] = runsOn | |||
| labels["job_name"] = *e.WorkflowJob.Name | |||
| labels["organization"] = *e.Repo.Owner.Login | |||
There was a problem hiding this comment.
@hamishforbes I realized #2218 added the organization label with another value of *e.Org.Name. If we call it "organization", that might be the correct value.
However, I do think it might be a good idea to be able to see *e.Repo.Owner.Login via labels when the workflow job event is coming from a runner associated with a repository owned by a user, not an organization.
That said, would you like to include *e.Repo.Owner.Login as another label, like "owner"?
mumoshu
force-pushed
the
feat/metrics-labels
branch
2 times, most recently
from
55d675a
to
c18e2c7
Compare
|
@hamishforbes Hey! I've addressed my own comments in the lastest commit. The label is now |
mumoshu
changed the title
mumoshu
force-pushed
the
feat/metrics-labels
branch
from
c18e2c7
to
c7112ec
Compare
Looks good to me! I do think |
mumoshu
force-pushed
the
feat/metrics-labels
branch
from
c7112ec
to
e24b580
Compare
|
@hamishforbes Thanks for your review! Re |
|
Fixed some panics related to the logger and the metrics labels for the case we feed workflow jobs from a repository instead of an org. |
mumoshu
changed the title
There was a problem hiding this comment.
Looks like it's working well for me! Let me merge this and keep testing locally.
Thank you so much for your effort @hamishforbes!
Fixes #2176