metrics: Add additional operational excellence metrics #156
Conversation
chandankumar4
force-pushed
the
additional-metrics
branch
3 times, most recently
from
9dd9b29
to
2d09a1f
Compare
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
force-pushed
the
additional-metrics
branch
from
2d09a1f
to
dbe042d
Compare
chandankumar4
requested review from
xdevxy,
juliev0 and
afugazzotto
as code owners
Signed-off-by: chandankumar4 <[email protected]>
why this doesn't include the total number of ISB service synced, etc? |
| @@ -84,19 +85,24 @@ func NewNumaflowControllerRolloutReconciler( | |||
| kubectl kubeUtil.Kubectl, | |||
| customMetrics *metrics.CustomMetrics, | |||
| ) (*NumaflowControllerRolloutReconciler, error) { | |||
| stateCache := sync.NewLiveStateCache(rawConfig) | |||
| stateCache := sync.NewLiveStateCache(rawConfig, customMetrics) | |||
| newRawConfig := metrics.AddMetricsTransportWrapper(customMetrics, rawConfig) | |||
There was a problem hiding this comment.
why do we call this function here as opposed to from main.go?
There was a problem hiding this comment.
now I see in the issue it says we're counting these only for NumaflowControllerRollout, but i'm not sure why
There was a problem hiding this comment.
Numaflow Controller is through gitops sync
There was a problem hiding this comment.
Not that we need to hold up this PR for this, but why do we only want to count "Number of kubernetes requests executed during reconciliation" for Numaflow Controller and not for ISBService and Pipeline?
There was a problem hiding this comment.
yeah I think we would want all, @chandankumar4 can you create an follow up issue to track this?
There was a problem hiding this comment.
I have covered it in this PR
|
for any metrics that are particular to Numaflow Controller can we have "numaflow_controller" or "numaflow_ctlr" in the name? |
|
Why is |
Fixed, It was missed register that variable. @xdevxy |
chandankumar4
force-pushed
the
additional-metrics
branch
from
41742d5
to
3324bae
Compare
|
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
force-pushed
the
additional-metrics
branch
from
3324bae
to
5b06851
Compare
Signed-off-by: chandankumar4 <[email protected]>
I think we initially thought |
| @@ -23,6 +23,7 @@ require ( | |||
| ) | |||
|
|
|||
| require ( | |||
| github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 | |||
There was a problem hiding this comment.
why do we need this dependency?
There was a problem hiding this comment.
I'm using it here if we don't wanted to use then need to copy that fn here with their dependencies.
| return ctrl.Result{}, statusUpdateErr | ||
| } | ||
|
|
||
| r.customMetrics.ISBServicesSyncFailed.WithLabelValues().Inc() |
There was a problem hiding this comment.
nit: this could be done just once above if statusUpdateErr != nil
There was a problem hiding this comment.
For having it only once, how It will cover if statusUpdateErr!=nil and err != nil, it should be called twice?
| @@ -317,6 +327,7 @@ func (r *ISBServiceRolloutReconciler) processExistingISBService(ctx context.Cont | |||
| } | |||
|
|
|||
| isbServiceRollout.Status.MarkDeployed(isbServiceRollout.Generation) | |||
| r.customMetrics.ReconciliationDuration.WithLabelValues(ControllerISBSVCRollout, "update").Observe(time.Since(syncStartTime).Seconds()) | |||
There was a problem hiding this comment.
unfortunately, we don't know that this is an "update" vs a "no-op", do we?
There was a problem hiding this comment.
similarly here, if isbServiceNeedsUpdating {...}
There was a problem hiding this comment.
nit: sorry, but what do you think about enclosing it in the else block that's directly above it? I think then it's very clear that this statement goes with the dataLossPrevention=false case only and there will be less chance of messing it up later. Otherwise, I had to look closely at the dataLossPrevention=true code above to confirm that it was in fact returning and would't hit this.
Signed-off-by: chandankumar4 <[email protected]>
btw I have already remove the namespace label from metrics but do we also wanted to revert this change and allow multiple numaplane-controller in one namespace? |
chandankumar4
force-pushed
the
additional-metrics
branch
from
0518436
to
fe59610
Compare
|
I'm kind of thinking about in a future PR, I wonder if we should consider avoiding doing metrics in the inner code and trying to as much as possible do them in one place, like in |
Signed-off-by: chandankumar4 <[email protected]>
chandankumar4
force-pushed
the
additional-metrics
branch
from
fe59610
to
c9be19a
Compare
Right, we may need to revert that change in future, but if you remove the namespace label in this PR then no need to open an issue for it. The revert will be tracked by no downtime upgrade feature, Thanks! |
Fixes #97
Modifications
Add additional operational excellence metrics
metrics data
Verification