Support tagged metrics in local backend #6
Conversation
1 similar comment
metrics/local.go
Outdated
| // "name|tag1=value1|...|tagN=valueN", where tag names are | ||
| // sorted alphabetically. | ||
| func getKey(name string, tags map[string]string) string { | ||
| var keys []string |
metrics/local.go
Outdated
| for k := range tags { | ||
| keys = append(keys, k) | ||
| keys[i] = k | ||
| i++ |
There was a problem hiding this comment.
you can still use append, you just need to allocate with initial size: make([]string, 0, len(tags))
There was a problem hiding this comment.
haha remember we discussed this before? I thought the same but after running benchmarks, append is slower than direct indexing.
There was a problem hiding this comment.
that case was different, there you already had an index variable incremented as part of the loop. So it made the code cleaner and faster. Here I don't think it's faster, just uglier due to i being in the outside scope.
Anyway, fine either way, this isn't a performance critical piece.
There was a problem hiding this comment.
sure, I'll fix it here because I agree it does look cleaner. I'll also fix the flaky tests as part of this PR.
|
The 1.6 build is still failing |
| numGoroutines := runtime.NumGoroutine() | ||
| defer func() { | ||
| assert.Equal(t, numGoroutines, runtime.NumGoroutine(), "Leaked at least one goroutine.") | ||
| }() |
There was a problem hiding this comment.
why do you think this is a source of flakiness?
There was a problem hiding this comment.
my guess is the deferred localbackend.stop() and this defer func have a race condition. Ofc the localbackend.stop() will run first but the check that stops the actual go routine doesn't get called before this function gets called. I can't reproduce locally so it's just a guess.
There was a problem hiding this comment.
Looks like you're correct. The stop() function waits until the bg routine that's being stopped writes to the stopped channel, and then stop() exists. But the bg routine may still linger and trip the count.
We could do the test differently, without actually testing that the go-routine died, but only testing that the function has exited, e.g. by flipping an atomic var or a waitgroup.
There was a problem hiding this comment.
Do you think there's value in doing this? adding an atomic var/waitgroup just to ensure that the function exited?
There was a problem hiding this comment.
Well, I think it is useful to test that when you call stop() the backend actually stops. Since you're removing the go-routine count validation, you are removing that functional test, without replacement.
|
I think with the waitgroup in place you do want to remove the check for go-routines, because it is inherently prone to race conditions. |
#2