-
Notifications
You must be signed in to change notification settings - Fork 59
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
[Feat] Node Server Metrics Implementation #278
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #278 +/- ##
==========================================
- Coverage 76.33% 75.31% -1.03%
==========================================
Files 21 22 +1
Lines 1644 1730 +86
==========================================
+ Hits 1255 1303 +48
- Misses 289 325 +36
- Partials 100 102 +2 ☔ View full report in Codecov by Sentry. |
…etheus - implemented metrics storage for `NodePublishVolume` function
- moved functions to a seperate package
- undid changes in main.go
- fixed `ineffassign` error for variable `success`
…environment variable
…metrics server is running - updated port to 10252 as default
…n additional parameter
internal/driver/nodeserver.go
Outdated
volumeID := req.GetVolumeId() | ||
log.V(2).Info("Processing request", "volumeID", volumeID) | ||
|
||
ns.mux.Lock() | ||
defer ns.mux.Unlock() | ||
|
||
success := metrics.SuccessTrue |
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.
what is this success
variable indicating? That the controller was successful in publishing the volume(basically whatever task it was attempting)?
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.
Yes, the variable indicates if the function was completed. true
is completion with no errors. false
indicates that there was an error and it's trying to execute the function again until completion.
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 function completes on line 156 though. Or Am I missing something?
Also can we rename start
and success
in all the methods to something more descriptive?
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.
Yes, it does. So initially the success variable is set to true
, but on each failure it's set as false
and stored into prometheus's registrar. If the execution goes to line 156 without any errors, success will be true
meaning the function was completed.
Sure, I'll rename the variables.
- refactored `start` to `functionStartTime` - refactored `success` to `functionStatus` - added service to expose the metrics to prometheus - defaulted the node-server port to 10251 - updated documentation with the new helm command - updated daemonset to pick up the environment variables to enable the metrics
helm-chart/csi-driver/templates/csi-linode-controller-metrics.yaml
Outdated
Show resolved
Hide resolved
- removed functionStatus variable
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.
LGTM! Great work on this!
This PR is to get the metrics of the functions performed by the node server. The functions for which the metrics are implemented are:
NodePublishVolume
NodeUnpublishVolume
NodeStageVolume
NodeUnstageVolume
NodeExpandVolume
General:
Pull Request Guidelines: