Conversation
black-adder
requested review from
pavolloffay,
vprithvi and
yurishkuro
as code owners
ghost
assigned
cmd/collector/app/metrics.go
Outdated
| counts: make(map[string]metrics.Counter), | ||
| factory: factory.Namespace("debug-spans."+category, nil), | ||
| lock: &sync.Mutex{}, | ||
| counts: make(map[string]metrics.Counter), |
There was a problem hiding this comment.
I refactored this slightly. Rather than having debugSpans and debugTraces as separate metrics, they should be part of the spans and traces metrics but with a debug tag. This way it's easier to aggregate. I guess technically this is a breaking change?
While not in scope of this PR, we could break up the taggify the metrics further by splitting out service as a tag.
|
change looks good, but I am concerned how this would overlap with #776 |
|
he who lands first, wins |
cmd/collector/app/metrics.go
Outdated
| }) | ||
| } | ||
| m.lock.Unlock() | ||
| if counter != nil { |
There was a problem hiding this comment.
we should have a fallback "all other services" name when over limit.
|
I'll address changes and keep this PR open, I'll coordinate with @jpkrohling so that these two changes can land at the same time. |
|
or might be easier to just do the changes in that PR |
|
do yo mean combine them or merge this and re-apply there? |
|
Ill redo this change on top of his branch and we can land it there |
Signed-off-by: Won Jun Jang <wjang@uber.com>
Signed-off-by: Won Jun Jang <wjang@uber.com>
black-adder
force-pushed
the
metric_for_root_traces
branch
from
1be203c to
ae24599
Compare
cmd/collector/app/metrics.go
Outdated
| } else { | ||
| counter = getOrCreateCounter(m.counts, func() metrics.Counter { | ||
| return m.factory.Counter("", map[string]string{"service": serviceName, "debug": "false"}) | ||
| }) |
There was a problem hiding this comment.
this is highly in efficient. The overall function is "count", one would expect it to be quick and zero-allocations, but you are creating all these lambdas that will require memory allocations.
There was a problem hiding this comment.
went from 1067 ns/op 231 B/op 11 allocs/op to 947 ns/op 165 B/op 9 allocs/op
There was a problem hiding this comment.
maybe if you benchmark the unhappy path, but try with the same service name in a hot loop
Signed-off-by: Won Jun Jang <wjang@uber.com>
cmd/collector/app/metrics.go
Outdated
| } | ||
| } | ||
|
|
||
| func (m *countsBySvc) x(serviceName, debugStr string, counts map[string]metrics.Counter) { |
There was a problem hiding this comment.
why do you need an extra function with such a great name? just set two local vars above and continue in the same function.
|
https://coveralls.io/jobs/36863391 <- 100% coverage. I'll investigate why overalls is being a pain but landing first. |
* master: (38 commits) Preparing release 1.5.0 (jaegertracing#847) Add bounds to memory storage (jaegertracing#845) Add metric for debug traces (jaegertracing#796) Change metrics naming scheme (jaegertracing#776) Bump gocql version (jaegertracing#829) Remove ParentSpanID from domain model (jaegertracing#831) Make gas run quiet (jaegertracing#838) Revert "Make gas run quite" Revert "Install gas from install-ci" Install gas from install-ci Make gas run quite Add 'gas' for security problems scanning (jaegertracing#830) Add ability to adjust static sampling probabilities per operation (jaegertracing#827) Support log-level flag on agent (jaegertracing#828) Remove unused function (jaegertracing#822) Add healthcheck to standalone (jaegertracing#784) Do not use KeyValue fields directly and use KeyValues as decorator only (jaegertracing#810) Add ContaAzul to the adopters list (jaegertracing#806) Add ISSUE_TEMPLATE and PULL_REQUEST_TEMPLATE (jaegertracing#805) Upgrade to go 1.10 (jaegertracing#792) ... # Conflicts: # cmd/agent/app/builder.go # cmd/collector/main.go # cmd/query/main.go # cmd/standalone/main.go
3 participants
Currently, we support metrics for the total number of debug spans but another useful metric to have is the debug traces (services that initiate the debug trace). Since debug traces aren't downsampled (this isn't technically true here in OSS...) having metrics to observe which service is creating more debug traces than necessary is useful.
Signed-off-by: Won Jun Jang wjang@uber.com