-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Restore runner name label in listener metrics #4186
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
base: master
Are you sure you want to change the base?
Restore runner name label in listener metrics #4186
Conversation
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.
Pull Request Overview
This PR restores the runner_name
label to job metrics as an opt-in feature that can be enabled through metrics configuration. The change addresses a community request to include runner identification in metrics for better observability.
- Adds
runner_name
label to job completion and startup metrics - Refactors label filtering logic by extracting a reusable helper function
- Updates configuration examples to demonstrate the new optional label
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
cmd/ghalistener/metrics/metrics.go | Adds runner_name label constant, updates job label methods to include runner name, and refactors label filtering into a helper function |
cmd/ghalistener/metrics/metrics_test.go | Adds comprehensive unit tests for the new label functionality and helper function |
charts/gha-runner-scale-set/values.yaml | Updates commented configuration examples to include the new runner_name label option |
ServerAddr: "", | ||
ServerEndpoint: "", | ||
Logger: logr.Discard(), | ||
Metrics: nil, // when metrics is nil, all default metrics should be registered |
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.
The test relies on implementation details by setting Metrics to nil and expecting specific behavior. Consider using a more explicit test setup that doesn't depend on nil handling.
Metrics: nil, // when metrics is nil, all default metrics should be registered | |
Metrics: NewDefaultMetrics(), // explicitly use default metrics implementation |
Copilot uses AI. Check for mistakes.
ServerAddr: "", | ||
ServerEndpoint: "", | ||
Logger: logr.Discard(), | ||
Metrics: nil, // when metrics is nil, all default metrics should be registered |
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.
The test relies on implementation details by setting Metrics to nil and expecting specific behavior. Consider using a more explicit test setup that doesn't depend on nil handling.
Metrics: nil, // when metrics is nil, all default metrics should be registered | |
Metrics: NewDefaultMetrics(), // explicitly configure default metrics |
Copilot uses AI. Check for mistakes.
@nikola-jokic |
This PR restores the
runner_name
label to job metrics as an opt-in feature for inspect pod performance issue purpose.The label can be enabled by configuring it in the metrics configuration.
Also adds unit tests that maintain existing structure and avoid complex mocking.
Origin: https://github.com/orgs/community/discussions/167243