-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Added node class to tagged metrics #3882
Added node class to tagged metrics #3882
Conversation
nice! |
@jippi hope it would be nice -19 ;) |
client/client.go
Outdated
if emittedNodeClass = c.config.Node.NodeClass; emittedNodeClass == "" { | ||
emittedNodeClass = "none" | ||
} | ||
c.baseLabels = []metrics.Label{ |
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.
Can you update to
c.baseLabels = []metrics.Label{{Name: "node_id", Value: c.NodeID()}, {Name: "datacenter", Value: c.Datacenter()}}
if node := c.Node(); node.NodeClass != "" {
c.baseLabels = append(c.baseLabels, {Name: "node_class", Value: node.NodeClass})
}
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.
sure! original idea was to add "none" like it's listed in CLI nomad node-status
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.
I like none
idea - having a sparse field for class
will be confusing :)
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.
we will not add the flag to report "none" client class, right?
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.
Problem is all possible strings you could add are going to be valid possible node classes
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.
@dadgar while working on nomad monitoring tutorial I ran into a problem with grouping nodes by class when node class is not emitted from not nodes with no class configured, so I reverted the code to my previous version when none is emitted from not tagged nodes.
The issue of all possible strings are going to be valid can be addressed on config parsing stage.
Let me know if you want me to add some restrictions there.
efd0e1d
to
2b8cb3f
Compare
@dadgar I rebased on master and made as you asked, but the build is failing on https://github.com/hashicorp/nomad/blob/master/nomad/structs/structs.go#L2966 |
2b8cb3f
to
d9bb450
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
For easier monitoring nomad cluster parts