Skip to content

Conversation

@TeonLucas
Copy link

@TeonLucas TeonLucas commented Nov 12, 2023

This adds a -local_only argument to support collecting local node metrics per node. The OHI would reside on each node, and thus propagates its host tags to the metrics collected. The master node would additionally collect cluster and common metrics with this option.

@TeonLucas TeonLucas requested a review from a team November 12, 2023 19:41
@CLAassistant
Copy link

CLAassistant commented Nov 12, 2023

CLA assistant check
All committers have signed the CLA.

Copy link
Member

Choose a reason for hiding this comment

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

mock: Unexpected Method Call
-----------------------------

Request(string)
		0: "/_nodes/_local/id"

The closest call I have is: 

Request(string)
		0: "/_nodes/_local"

Copy link
Author

Choose a reason for hiding this comment

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

need help updating mock

Copy link
Author

Choose a reason for hiding this comment

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

Updated integration tests so that it passes now

@paologallinaharbur paologallinaharbur requested a review from a team November 13, 2023 08:46
IndicesRegex string `default:"" help:"A regex pattern that matches the index names to collect. Collects all if unspecified"`
ShowVersion bool `default:"false" help:"Print build information and exit"`
MasterOnly bool `default:"false" help:"Collect cluster metrics on the elected master only"`
LocalOnly bool `default:"false" help:"Collect node metrics on the local node only"`
Copy link
Member

Choose a reason for hiding this comment

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

should we add an integration test?

Copy link
Author

Choose a reason for hiding this comment

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

need help with this

src/metrics.go Outdated
Comment on lines 79 to 82
} else {
needed = true
}
return
Copy link
Member

Choose a reason for hiding this comment

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

simply

 return true

Copy link
Author

Choose a reason for hiding this comment

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

cleaned up the function logic

src/metrics.go Outdated
log.Error("There was an error gathering the elected master node ID: %v", err)
return
}
needed = nodeID == masterID
Copy link
Member

Choose a reason for hiding this comment

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

Also here I prefer an early
return nodeID == masterID

and maybe without named return values is 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

src/metrics.go Outdated
if args.MasterOnly || args.LocalOnly {
masterID, err := getMasterNodeID(client)
if err != nil {
log.Error("There was an error gathering the elected master node ID: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

We should not hide errors, The signature likely should be
func getClusterNeeded(client Client, nodeID string) (bool, Error) {

But then it is again difficult to read 🤔

Copy link
Author

Choose a reason for hiding this comment

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

updated function name

@TeonLucas
Copy link
Author

Still need to add a test for -local_only but otherwise make test passes now.

@TeonLucas
Copy link
Author

Looks like returntocorp/semgrep-action is deprecated, causing static analysis check to fail.

@paologallinaharbur
Copy link
Member

You can get rid of returntocorp/semgrep-action

We did the same here

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.

3 participants