Reimplement expvar metrics to be tolerant of duplicates #40
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
======================================
Coverage 100% 100%
======================================
Files 21 25 +4
Lines 595 710 +115
======================================
+ Hits 595 710 +115
Continue to review full report at Codecov.
|
metrics/adapters/cache.go
Outdated
| r.lock.Lock() | ||
| defer r.lock.Unlock() | ||
| r.timers[name] = t | ||
| } |
There was a problem hiding this comment.
It's times like these that make me miss overloading functions and generics/templates. I'm assuming there is no way Go would support a single get/set.
metrics/adapters/tagless.go
Outdated
|
|
||
| import "github.com/uber/jaeger-lib/metrics" | ||
|
|
||
| // FactoryWithoutTags creates metrics with fully qualified name and tags. |
metrics/adapters/factory.go
Outdated
|
|
||
| func (f *factory) Counter(name string, tags map[string]string) metrics.Counter { | ||
| fullName, fullTags, key := f.getKey(name, tags) | ||
| c, ok := f.cache.getCounter(key) |
There was a problem hiding this comment.
There maybe a race condition here. If 2 threads get here at the same time, they both will see nil counter and then both proceed to create a counter and only one is saved in the cache.
There was a problem hiding this comment.
Indeed, good catch. We could do a getOrSet() passing a lambda to create if not found. Certainly for metrics related to service names we could easily see a race condition.
There was a problem hiding this comment.
Why not do what Isaac recommended in other PRs and make the cache a lock free data structure and have the factories, etc. do the locking? This isn't that performance intensive anyway.
There was a problem hiding this comment.
if the cache is lock-free, then the locking has to be in the factory, but factory gets recreated every time you call factory.Namespace(), and they all share the same cache. I think making cache thread-safe gives better encapsulation in this case.
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
|
@black-adder comments addressed |
Required for jaegertracing/jaeger#716
Add a new module
metrics/adaptersthat provide caching of the created metrics objects and the common functionality of managing namespaces and flattening tags into metric id string.