Skip to content
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

fix(exporter): Handle pool sync time metrics collection gracefully #1616

Merged
merged 2 commits into from
Feb 26, 2020

Conversation

mittachaitu
Copy link

@mittachaitu mittachaitu commented Feb 14, 2020

Signed-off-by: mittachaitu [email protected]

What this PR does / why we need it:
This PR fixes a bug in the pool sync time metrics collector in Maya-exporter.
If the last request is in progress, don't queue the current request during pool
last Sync time metrics collection and increment the reject request counter.
Note
This PR is the address comments of #1615.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
UnitTest will be added once after understanding the flow.

Checklist:

  • Fixes #
  • Labelled this PR & related issue with documentation tag
  • PR messages has document related information
  • Labelled this PR & related issue with breaking-changes tag
  • PR messages has breaking changes related information
  • Labelled this PR & related issue with requires-upgrade tag
  • PR messages has upgrade related information
  • Commit has unit tests
  • Commit has integration tests

This PR fixes a bug in pool sync time metrics collector in maya exporter.
If last request is in progress, don't queue the current request during pool
last Sync time metrics collection and increament the reject request counter.

Signed-off-by: mittachaitu <[email protected]>
@@ -68,6 +68,7 @@ type poolSyncMetrics struct {
zpoolLastSyncTime *prometheus.GaugeVec
zpoolStateUnknown *prometheus.GaugeVec
zpoolLastSyncTimeCommandError *prometheus.GaugeVec
zpoolListRequestRejectCounter prometheus.Gauge
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using prometheus.Gauge, using promethus.Counter will be better.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gave one comment regarding use of Counter. Otherwise lgtm

Copy link
Contributor

@utkarshmani1997 utkarshmani1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a minor comment about name, PTAL

prometheus.GaugeOpts{
Namespace: "openebs",
Name: "zpool_list_request_reject_count",
Help: "Total no of rejected requests of zpool list",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should be actual command separated by underscore

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL

if p.isRequestInProgress() {
p.zpoolListRequestRejectCounter.Inc()
p.Unlock()
p.zpoolListRequestRejectCounter.Collect(ch)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will the above line (line no. 121) do exactly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Collect is called by the Prometheus registry when collecting
metrics. The implementation sends each collected metric via the
provided channel and returns once the last metric has been sent. The
descriptor of each sent metric is one of those returned by Describe
(unless the Collector is unchecked, see above). Returned metrics that
share the same descriptor must differ in their variable label values.

The above statement is taken Prometheus docs/comments.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@utkarshmani1997 utkarshmani1997 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@singhmeghna79
Copy link
Contributor

LGTM

Copy link
Contributor

@vishnuitta vishnuitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes are good

@vishnuitta vishnuitta merged commit 9417d96 into openebs-archive:master Feb 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants