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

up metrics continue 1 when aci node is unreachable. #70

Closed
wants to merge 1 commit into from

Conversation

minefuto
Copy link
Contributor

@minefuto minefuto commented Nov 1, 2024

up metrics continue 1 when aci node is unreachable(e.g. aci node down) using "node based queries".
I think if aci node is unreachable, up metrics shoud be goes to 0.
I added node probe per scraping.
The node probe checks aci-exporter can access to aci node or not by http request.

@thenodon
Copy link
Member

thenodon commented Nov 3, 2024

Tanks for the PR @minefuto. I agree that if the node is down it should return up=0, but your code will add one extra http call on every request, which I do not like if it can be avoided. If an error happen at the GET request, like from func (p aciAPI) getClassMetrics nil is returned on the channel that expect []MetricDefinition, so I think it could be resolved by logic that check for nil in the loop

for i := 0; i < 4; i++ {
.
Something like

	for i := 0; i < 4; i++ {
		result := <-ch
		if result != nil {
			metrics = append(metrics, result...)
		} else {
                    // handle down
		}
	}

I you have the time to give it a try it would be great.

Copy link

sonarqubecloud bot commented Nov 7, 2024

@minefuto
Copy link
Contributor Author

minefuto commented Nov 7, 2024

Thanks feedback @thenodon. I understand.

I think it could be resolved by logic that check for nil in the loop

I think if one of the query configuration(class, group class, compound) is empty, up metrics goes to 0 because empty configuration query return nil.
So I implemented up metrics goes to 0 if all queries return nil.
Could you please feedback again?

@thenodon
Copy link
Member

thenodon commented Nov 7, 2024

@minefuto the way you have done it now logging and the timer metrics will be lost, so it will not be consistent. If you want you can make an issue and I can implement it.

@minefuto
Copy link
Contributor Author

OK. I will open issue.
Thanks @thenodon .

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.

2 participants