-
Notifications
You must be signed in to change notification settings - Fork 793
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 cache metrics #88
add cache metrics #88
Conversation
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.
Hi Matsumana,
thanks for you contribution. I added some comments to the code, mainly asking you to stick to the pattern we are already using. The additional type nodeMetricVec and its usage makes the code really hard to read.
collector/nodes.go
Outdated
defaultThreadPoolLabels = append(defaultNodeLabels, "type") | ||
defaultBreakerLabels = append(defaultNodeLabels, "breaker") | ||
defaultFilesystemLabels = append(defaultNodeLabels, "mount", "path") | ||
defaultCacheCategoryLabels = append(defaultNodeLabels, "category") |
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.
a don't think "category" describes the label correctly. I would then prefer "cache"
collector/nodes.go
Outdated
defaultFilesystemLabels = append(defaultNodeLabels, "mount", "path") | ||
defaultCacheCategoryLabels = append(defaultNodeLabels, "category") | ||
defaultCacheHitLabels = append(defaultNodeLabels, "hit") | ||
defaultCacheMissLabels = append(defaultNodeLabels, "miss") |
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.
"hit" and "miss" are the labelvalues for defaultCacheCategoryLabels
, not labels. So it really confuses the code reader if they appear in a slice together with labels
collector/nodes.go
Outdated
LabelValues func(cluster string, node NodeStatsNodeResponse) [][]string | ||
Values func(node NodeStatsNodeResponse) []float64 | ||
} | ||
|
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 don't see a reason adding a new type for metrics. The type nodeMetric
already covers the usecase. Please use it. It is also pretty unclear what the dimensions of LabelValues func(...) [][]string
represent. Same for Values func(...) []float64
. A metric label can't be a slice
collector/nodes.go
Outdated
values := metric.Values(node) | ||
|
||
for i, labelValue := range labelValues { | ||
counterVec := prometheus.NewCounterVec(prometheus.CounterOpts{ |
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.
NewCounterVec
must not be called in the Collect
func implementing prometheus.Collector
. Throughout the collector we are using the pattern to create a new const metric. (prometheus.MustNewConstMetric
). It perfectly fits to our use case.
collector/nodes.go
Outdated
counterVec.WithLabelValues(labelValue...).Set(values[i]) | ||
counterVec.Collect(ch) | ||
} | ||
|
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.
this metric returns false results regarding the labels: on our cluster we get:
# TYPE elasticsearch_indices_query_cache_count counter
elasticsearch_indices_query_cache_count{category="hit",cluster="cluster",host="host",name="name"} 3.1548049e+07
elasticsearch_indices_query_cache_count{category="miss",cluster="cluster",host="host",name="name"} 4.9529376e+07
(the label values for host and name are not (!) redacted)
@zwopir Thank you for the comments. |
yes, that looks good - Thanks again for your contribution! |
Hello.
With reference to #87, I added metrics the following.
Please review.