Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bugfix] apm.DefaultTracer misbehaves when transport configuration is invalid #1618

Merged
merged 10 commits into from
May 6, 2024
Merged
8 changes: 3 additions & 5 deletions tracer.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ var (
// DefaultTracer returns the default global Tracer, set the first time the
// function is called, or after calling SetDefaultTracer(nil).
//
// The default tracer is configured via environment variables, and will always
// be non-nil. If any of the environment variables are invalid, the
// corresponding errors will be logged to stderr and the default values will be
// used instead.
// The default tracer is configured via environment variables, and if the those are invalid
// the tracer will be disabled.
func DefaultTracer() *Tracer {
tracerMu.RLock()
if defaultTracer != nil {
Expand Down Expand Up @@ -314,7 +312,7 @@ func (opts *TracerOptions) initDefaults(continueOnError bool) error {
if opts.Transport == nil {
initialTransport, err := initialTransport(opts.ServiceName, opts.ServiceVersion)
if failed(err) {
opts.Transport = transport.NewDiscardTransport(err)
active = false
} else {
opts.Transport = initialTransport
}
Expand Down
13 changes: 4 additions & 9 deletions tracer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -597,17 +597,12 @@ func TestTracerDefaultTransport(t *testing.T) {
require.Error(t, err)
assert.EqualError(t, err, "failed to parse ELASTIC_APM_SERVER_TIMEOUT: invalid duration never")

// Implicitly created Tracers will have a discard tracer.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend running the go linter locally. This additional new line is not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will also want to run the tests locally, as they currently are failing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @dmathieu, I did run test before raising the PR and again. As per the code DefaultTracer is being used in three places and I ran the UT's and they are working fine
Screenshot 2024-05-04 213502

Copy link
Member

@dmathieu dmathieu May 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make test did fail consistently in CI, as well as locally here.
This was because the tracecontext example relies on a discarded tracer.
But now, it had an inactive one (though this was due to previous tests). I've fixed it.

// Implicitly created Tracers will have Inactive tracer incase of invalid configuration
dmathieu marked this conversation as resolved.
Show resolved Hide resolved
apm.SetDefaultTracer(nil)
tracer = apm.DefaultTracer()
dmathieu marked this conversation as resolved.
Show resolved Hide resolved

tracer.StartTransaction("name", "type").End()
tracer.Flush(nil)
assert.Equal(t, apm.TracerStats{
Errors: apm.TracerStatsErrors{
SendStream: 1,
},
}, tracer.Stats())
isActiveTracer :=tracer.Active()
assert.Equal(t,false,isActiveTracer)
dmathieu marked this conversation as resolved.
Show resolved Hide resolved
})
}

Expand Down
Loading