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

ACCDT-1239: reduce cardinality of metrics #4

Merged
merged 3 commits into from
Nov 15, 2023

Conversation

sindunuragarp
Copy link

@sindunuragarp sindunuragarp commented Nov 14, 2023

Motivation

[ACCDT-1239]

Currently the volume of metrics is too high due to cardinality. This was remedied before by using metric relabelings, but there are benefits of applying the change directly in the producing code.

Proposed Changes

  • Only store job duration metrics when the conclusion is relevant
  • Drop head branch and repository full name

@sindunuragarp sindunuragarp marked this pull request as ready for review November 14, 2023 17:49
@sindunuragarp sindunuragarp requested a review from a team as a code owner November 14, 2023 17:49
@@ -64,6 +64,10 @@ func (reader *EventReader) ProcessWorkflowJobEvent(ctx context.Context, event in
keysAndValues = []interface{}{"job_id", fmt.Sprint(*e.WorkflowJob.ID)}
)

if len(e.WorkflowJob.Labels) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

have you checked, what is the cases when we have this empty? just to understand what we are dropping?

Copy link
Author

@sindunuragarp sindunuragarp Nov 15, 2023

Choose a reason for hiding this comment

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

@kyrylomiro I looked into it a bit, and so far, no run labels appear when no runner was started for the job. This happens when:

  • the job calls a reusable workflow. the root job doesn't have runners, but sub jobs in the workflow appears and have runners
  • the job has an if statement that wasn't fulfilled
  • the job wasn't triggered, due to previous dependency
  • it is a check added by an app, and not an actual job (e.g. miro:version)

So I think it should be okay to skip over them. I will do a bit more deep dive after lunch

Copy link
Author

Choose a reason for hiding this comment

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

Looks like the list above is accurate. So it should be okay to skip those entries for the metrics.

@sindunuragarp sindunuragarp merged commit db13bb6 into master Nov 15, 2023
5 checks passed
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