Labeling overhaul #118
Labeling overhaul #118
Conversation
| @@ -16,7 +16,7 @@ GOPATH := $(firstword $(subst :, ,$(shell $(GO) env GOPATH))) | |||
| PROMU ?= $(GOPATH)/bin/promu | |||
| GOLINTER ?= $(GOPATH)/bin/gometalinter | |||
| #aligncheck and gosimple took unfortunately too long at travisCI | |||
| GOLINTER_OPT ?= --vendor --deadline 6m --cyclo-over=19 --disable=aligncheck --disable=gosimple | |||
There was a problem hiding this comment.
I still think it would be great if we could disable the cyclo-check for the *test.go files, but I'm not sure that's possible... C'est la vie.
There was a problem hiding this comment.
Perhaps a separate contribution to the metalinter project? :P
lustre_exporter_test.go
Outdated
| } | ||
| } | ||
|
|
||
| return 0, errors.New("Strings are too similar") |
There was a problem hiding this comment.
Just curious, if you have two identical strings/labels, does the test break? ie. Say you have a [bad] metric of metric{"name": "metric", "name": "metric"}, does it fail in this scenario? I think I'm a little confused as to this line... I believe you that it would be necessary, I'm just struggling to think of a scenario, so I want to learn more.
There was a problem hiding this comment.
Regardless of if I could write the code to make it pass, we're in a bad situation if we have duplicate labels...so I just decided to return an error. It should fail, yes. Perhaps I should write unit tests...for unit tests... >_>
lustre_exporter_test.go
Outdated
| @@ -137,1159 +186,1167 @@ func blacklisted(blacklist []string, metricName string) bool { | |||
| } | |||
|
|
|||
| func TestCollector(t *testing.T) { | |||
| targets := []string{"OST", "MDT", "MGS", "MDS", "Client", "Generic", "LNET"} | |||
| targets := []string{"MGS", "OST", "MDT", "MDS", "Client", "Generic", "LNET"} | |||
There was a problem hiding this comment.
Again, just curious, is there a reason that the order was changed? I don't have a problem with it but would like to know if it changes the functionality in any way. I'm assuming you switched the order here to test on a smaller sample size, yes? I think I recall you saying that...
There was a problem hiding this comment.
I switched them for testing purposes (was impossible to figure out what I was doing wrong when testing against the OST data first). I can switch it back.
lustre_exporter_test.go
Outdated
| {"lustre_exports_pending_total", "Total number of exports that have been marked pending", counter, []labelPair{{"ost", "lustrefs-OST0001"}}, 2.513043456e+10, false}, | ||
| {"lustre_recalc_freed_total", "Number of locks that have been freed", counter, []labelPair{{"ost", "lustrefs-OST0001"}}, 0, false}, | ||
| {"lustre_lock_contended_total", "Number of contended locks", counter, []labelPair{{"ost", "lustrefs-OST0001"}}, 32, false}, | ||
| {"lustre_recovery_time_hard_seconds", "Maximum timeout 'recover_time_soft' can increment to for a single server", gauge, []labelPair{{ |
There was a problem hiding this comment.
I know you were thinking of this already, but "host" might not be the best descriptor here since "lustrefs-OST0000" is the filesystem and target names. Maybe "target" is better, but that might not be perfect when values of "osd" come in to play... I'm not sure what the best answer is here...
There was a problem hiding this comment.
No rush to merge this PR, so we'll take some time to figure out the right answer here.
joehandzik
force-pushed
the
wip-normalize-labels
branch
2 times, most recently
from
7b7eab0 to
0d02c61
Compare
Convert Generic to generic for generic metric labels. Realign labeling to share common label names across the board. Instead of MDS, MGS, OST, etc having different labels, they all have "component" and "host" labels now. Signed-Off-By: Joe Handzik <joseph.t.handzik@hpe.com>
joehandzik
force-pushed
the
wip-normalize-labels
branch
from
0d02c61 to
90980c3
Compare
|
@roclark Sure, may as well. We're still below 1.0, so we can change the "target" name later if someone hates it. |
|
Works for me! |
Convert Generic to generic for generic metric labels.
Realign labeling to share common label names across the board. Instead of MDS, MGS, OST, etc having different labels, they all have "component" and "host" labels now.
Signed-Off-By: Joe Handzik joseph.t.handzik@hpe.com
Fixes #100