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

Add metrics labels to all metrics #270

Merged
merged 4 commits into from
Aug 8, 2022
Merged

Conversation

huikang
Copy link
Contributor

@huikang huikang commented Aug 2, 2022

Cherry pick from the metrics-label-backport branch

@huikang huikang requested a review from rboyer August 2, 2022 14:51
config.go Outdated
// CIDRsAllowed If nil, allow any connection (default), otherwise specify all networks
// allowed to connect (you must specify IPv6/IPv4 separately)
// Using [] will block all connections.
CIDRsAllowed []net.IPNet

// MetricLabels is a map of optional labels to apply to all metrics emitted.
MetricLabels map[string]string
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to match the serf config and have this be a slice of metric labels?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way looks good to me. If we switch to MetricLabels []metrics.Label, we can get rid of mapToLabels and pass []metrics.Label all the way from consul to memberlist, right?

Copy link
Member

Choose a reason for hiding this comment

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

yep

memberlist.go Outdated
@@ -198,6 +203,8 @@ func newMemberlist(conf *Config) (*Memberlist, error) {
}
}

metricLabels := conf.MetricLabels
Copy link
Member

Choose a reason for hiding this comment

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

nit: why make a local variable for two uses below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I forgot to delete the variables after change the type; we used to have metricLabels := mapToLabels(conf.MetricLabels)

Fixed now.

@huikang huikang merged commit e6ff9b2 into master Aug 8, 2022
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