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

feat: autoresolve ems would export metric value as 0 and autoresolve=true label #2120

Merged
merged 5 commits into from
Jun 12, 2023

Conversation

Hardikl
Copy link
Contributor

@Hardikl Hardikl commented Jun 1, 2023

image

@cgrinds
Copy link
Collaborator

cgrinds commented Jun 5, 2023

We should update the documentation to define the autoresolve label

@Hardikl
Copy link
Contributor Author

Hardikl commented Jun 5, 2023

We should update the documentation to define the autoresolve label

Added doc changes for this.

docs/configure-ems.md Outdated Show resolved Hide resolved
@@ -41,6 +42,7 @@ type Ems struct {
eventNames []string // consist of all ems events supported
bookendEmsMap map[string]*set.Set // This is reverse bookend ems map, [Resolving ems]:[Set of Issuing ems]. Using Set here to ensure that it has slice of unique issuing ems
resolveAfter map[string]time.Duration // This is resolve after map, [Issuing ems]:[Duration]. After this duration, ems got auto resolved.
autoresolved bool
Copy link
Contributor

Choose a reason for hiding this comment

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

How can a single variable autoresolve is handling multiple matrix containing EMS messages? This single var state seems wrong at Collector level. This should be at ems level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, it's not needed as it would export e.Matrix with instance exported=true/false and work as expected.

@Hardikl
Copy link
Contributor Author

Hardikl commented Jun 6, 2023

Few corrections:

  • InstanceCount we show all instances earlier, but should be only which are exported=true
  • when records=0, we put count and instances as 0, which would be true in this code changes as well because HandleResult for loop won't be executed.

Copy link
Collaborator

@cgrinds cgrinds left a comment

Choose a reason for hiding this comment

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

will the logging issues we discussed in earlier meeting be handled in this PR or a different one?

@@ -652,7 +643,16 @@ func (e *Ems) HandleResults(result []gjson.Result, prop map[string][]*emsProp) (
count += instanceLabelCount
}
}
return m, count

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we calculate instanceCount in the above loops above, so we don't have to loop through all instances of all matrices again here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's added here mainly for autoresolve case.
In that case, we haven't get any records from ONTAP, so it won't run the above loop, but still we want to export 1 instance which would be resolved based on time.

@Hardikl
Copy link
Contributor Author

Hardikl commented Jun 9, 2023

will the logging issues we discussed in earlier meeting be handled in this PR or a different one?

Logging issue is resolved in this PR at 404-407 lines.
It will be logged only exportable instance count not all belongs to e.Matrix.

@Hardikl
Copy link
Contributor Author

Hardikl commented Jun 9, 2023

Matches label counts when multiple ems with same name exist would be handled in next PR

@cgrinds cgrinds merged commit a57a6f4 into main Jun 12, 2023
@cgrinds cgrinds deleted the hl_ems_autoresolve branch June 12, 2023 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When resolve_after time expires, Harvest should set ems_events metric value to 0
3 participants