stats: Add tally reporter to emit tagged metrics #676
Conversation
This is an open-sourcing of the tally StatsReporter which will emit tagged metrics rather than statsd metrics.
Codecov Report
@@ Coverage Diff @@
## dev #676 +/- ##
==========================================
+ Coverage 86.48% 86.76% +0.27%
==========================================
Files 39 39
Lines 3958 3958
==========================================
+ Hits 3423 3434 +11
+ Misses 409 400 -9
+ Partials 126 124 -2
Continue to review full report at Codecov.
|
| @@ -1,14 +1,16 @@ | |||
| hash: d6093eb1a6d4c1eaa9f2ad996ebf07748b0b3a0413f4ab9b9435df1d255567a5 | |||
| updated: 2017-09-25T17:19:51.449837394-07:00 | |||
| hash: b75ed7f632cfd185b6a8f8bc037b09d32e292f908af6c1e257e444e17a3a39ef | |||
There was a problem hiding this comment.
tip: I’ve found that it’s easier to rebase if I keep glide.lock updates in their own commit. Easy to remove the commit when I rebase, then recreate with glide up, instead of dealing with glide.lock merge conflicts. More relevant on a more active repository.
There was a problem hiding this comment.
Yep, agreed. Since this repo isn't really updated, this hasn't been an issue.
| @@ -0,0 +1,188 @@ | |||
| package stats | |||
There was a problem hiding this comment.
Might be worth stuttering this package name, e.g., tchanneltally.
There was a problem hiding this comment.
This is adding to the existing tchannel stats package:
https://godoc.org/github.com/uber/tchannel-go/stats
stats/tally.go
Outdated
| // NewTallyReporter takes a tally.Scope and wraps it so it ca be used as a | ||
| // StatsReporter. The list of metrics emitted is documented on: | ||
| // https://tchannel.readthedocs.io/en/latest/metrics/ | ||
| // The tags emitted are similar to Muttley/YARPC, the tags emitted are: |
There was a problem hiding this comment.
Public mention of Muttley.
| kt := convertTags(tags) | ||
|
|
||
| w.RLock() | ||
| ts, ok := w.byTags[kt] |
There was a problem hiding this comment.
In YARPC’s observability layer, we often digest the tags into a null-delimited string. You know the performance of struct key map lookup better than me. This is on the hot path?
There was a problem hiding this comment.
Good question -- this is definitely on the hot path (since we emit multiple stats per RPC).
A single string lookup is faster than using a struct with 4 strings. Just the lookup has a huge performance difference (the struct has 4 strings, and the string is the 4 strings concatenated with a null byte separator):
string-lookup: 24.6 ns/op
struct-lookup: 84.4 ns/op
However, this is not enough to look at, as using a single string requires building that string. Let's add the cost of building a string (without allocating) by reusing a shared buffer, and then do the lookup:
string-build-lookup: 63.8
This may be called from multiple goroutines, so we can't reuse a single buffer. Once we add buffer pooling, the cost difference is negligible, and the complexity of pooling (and risk of issues) is probably not worth the optimization.
This is an open-sourcing of the tally StatsReporter which will emit
tagged metrics rather than statsd metrics.