-
Notifications
You must be signed in to change notification settings - Fork 98
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
Add New Metric to Show Number of Running PipelineRuns #1771
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1771 +/- ##
==========================================
- Coverage 65.16% 64.91% -0.25%
==========================================
Files 174 175 +1
Lines 13250 13363 +113
==========================================
+ Hits 8634 8675 +41
- Misses 4043 4108 +65
- Partials 573 580 +7 ☔ View full report in Codecov by Sentry. |
901aa0d
to
3efd1df
Compare
pkg/metrics/metrics.go
Outdated
@@ -19,6 +26,10 @@ var prDurationCount = stats.Float64("pipelines_as_code_pipelinerun_duration_seco | |||
"number of seconds all pipelineruns completed in by pipelines as code", | |||
stats.UnitDimensionless) | |||
|
|||
var runningPRCount = stats.Float64("pipelines_as_code_running_pipelineruns_count", | |||
"number of running pipeline runs by pipelines as code", |
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.
"number of running pipeline runs by pipelines as code", | |
"number of running pipelineruns runs by pipelines as code", |
pkg/metrics/metrics.go
Outdated
ReportingPeriod: 30 * time.Second, | ||
} | ||
// Default to 10s intervals. | ||
ReportingPeriod: 10 * time.Second, |
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.
how did you choose this number?
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.
@chmouel oh, that is mistakenly.
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.
Actually I was running some pipelineruns to get metrics which were finishing in 20 seconds and original interval period was 30 seconds so injection code wasn't running for last pipelineruns and metric was having 1 only (it was being updated to 0 after all PR has done) that's why I changed it to 10 seconds, but I don't think that in production there would be any PipelineRun which will ends in 20 seconds.
added new gauge metric to show number of running pipelineruns. added tests and docs accordingly. https://issues.redhat.com/browse/SRVKP-6375 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
/test |
1 similar comment
/test |
added new gauge metric to show number of running pipelineruns. added tests and docs accordingly.
https://issues.redhat.com/browse/SRVKP-6375
Changes
Submitter Checklist
📝 Please ensure your commit message is clear and informative. For guidance on crafting effective commit messages, refer to the How to write a git commit message guide. We prefer the commit message to be included in the PR body itself rather than a link to an external website (ie: Jira ticket).
♽ Before submitting a PR, run make test lint to avoid unnecessary CI processing. For an even more efficient workflow, consider installing pre-commit and running pre-commit install in the root of this repository.
✨ We use linters to maintain clean and consistent code. Please ensure you've run make lint before submitting a PR. Some linters offer a --fix mode, which can be executed with the command make fix-linters (ensure markdownlint and golangci-lint tools are installed first).
📖 If you're introducing a user-facing feature or changing existing behavior, please ensure it's properly documented.
🧪 While 100% coverage isn't a requirement, we encourage unit tests for any code changes where possible.
🎁 If feasible, please check if an end-to-end test can be added. See README for more details.
🔎 If there's any flakiness in the CI tests, don't necessarily ignore it. It's better to address the issue before merging, or provide a valid reason to bypass it if fixing isn't possible (e.g., token rate limitations).