-
Notifications
You must be signed in to change notification settings - Fork 289
Conversation
Signed-off-by: Yuri Shkuro <ys@uber.com>
Codecov Report
@@ Coverage Diff @@
## master #447 +/- ##
=======================================
Coverage 84.69% 84.69%
=======================================
Files 33 33
Lines 1503 1503
=======================================
Hits 1273 1273
Misses 168 168
Partials 62 62 Continue to review full report at Codecov.
|
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
tracer.go
Outdated
t.metrics.SpansStartedDelayedSampling.Inc(1) | ||
if newTrace { | ||
t.metrics.TracesStartedDelayedSampling.Inc(1) | ||
} else if sp.firstInProcess { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SA9003: empty branch (from staticcheck
)
@@ -93,7 +93,7 @@ func (s *tracerSuite) TestBeginRootSpan() { | |||
s.NotNil(ss.duration) | |||
|
|||
s.metricsFactory.AssertCounterMetrics(s.T(), []metricstest.ExpectedMetric{ | |||
{Name: "jaeger.tracer.finished_spans", Value: 1}, | |||
{Name: "jaeger.tracer.finished_spans", Tags: map[string]string{"sampled": "y"}, Value: 1}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we may want to tighten these tests by also asserting the length of these metrics to verify that nothing additional is emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that'd only make the tests more fragile. The objective here is to test that some specific metrics are emitted.
Signed-off-by: Yuri Shkuro <ys@uber.com>
Part of # #449