[YUNIKORN-2818] Fix state tracking metrics for app and queue #951
Conversation
pkg/metrics/scheduler.go
Outdated
| @@ -84,8 +84,8 @@ func InitSchedulerMetrics() *SchedulerMetrics { | |||
| Help: "Total number of attempts to allocate containers. State of the attempt includes `allocated`, `rejected`, `error`, `released`", | |||
| }, []string{"state"}) | |||
|
|
|||
| s.applicationSubmission = prometheus.NewCounterVec( | |||
| prometheus.CounterOpts{ | |||
| s.applicationSubmission = prometheus.NewGaugeVec( | |||
There was a problem hiding this comment.
Change to gauge because, we want to support dec also
There was a problem hiding this comment.
A submission can not be changed. When an application is submitted it will always be submitted. It should be a counter that keeps increasing. It is not a state that we track it is a pure counter from start to shutdown.
There was a problem hiding this comment.
Thanks @wilfred-s for clarify for this field,it makes sense to me.
zhuqi-lucas
requested review from
wilfred-s,
pbacsko,
chenyulin0719 and
craigcondit
and removed request for
pbacsko
|
Please run |
Fix go lint now. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #951 +/- ##
==========================================
+ Coverage 79.56% 80.18% +0.61%
==========================================
Files 97 97
Lines 12275 12371 +96
==========================================
+ Hits 9767 9920 +153
+ Misses 2231 2176 -55
+ Partials 277 275 -2 ☔ View full report in Codecov by Sentry. |
pkg/metrics/queue.go
Outdated
| func (m *QueueMetrics) DecQueueApplicationsRejected() { | ||
| m.decQueueApplications(AppRejected) | ||
| } |
There was a problem hiding this comment.
DecQueueApplicationsRejected() is not called in "leave_Rejected" state? Is it intentional?
There was a problem hiding this comment.
Rejected applications should only increase, never decrease: a simple counter.
It is a final state that the application only gets removed from as part of the cleanup. We keep it around for a while to make it traceable for the submitter. i.e. submit an application that gets rejected: find it is the list as such. If we would drop it immediately the submitter has no idea what happened.
There was a problem hiding this comment.
@chenyulin0719 @wilfred-s
If i make sense right, we don't need to add decrease for following state? Because they are final state besides the expired:
- Rejected
- Failed
- Completed
And for expired, i don't think we need to tracking it?
There was a problem hiding this comment.
Addressed in latest PR.
There was a problem hiding this comment.
@wilfred-s Understood, thanks for the explanation.
@zhuqi-lucas Not tracking expired app metrics make sense to me.
To avoid any confusion, I think we should remove all the decrement function for Rejected/Failed/Completed. WDYT?
There was a problem hiding this comment.
I mean removing the below functions:
- DecQueueApplicationsRejected()
- DecQueueApplicationsFailed()
- DecQueueApplicationsCompleted()
There was a problem hiding this comment.
Got it @chenyulin0719 , remove those unused code in latest PR!
Good catch, thanks @chenyulin0719 for review! |
craigcondit
changed the title
What is this PR for?
State of appMetrics of Queue Metrics is incomplete and should be fixed
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-2818
How should this be tested?
Screenshots (if appropriate)
Questions: