feat(trace): add concurrent-safe Reset method to SpanRecorder

[flc1125]

We probably need a Reset method to reuse it for testing. Just like InMemoryExporter.

opentelemetry-go/sdk/trace/tracetest/exporter.go

Lines 61 to 65 in b99d2b8

	func (imsb *InMemoryExporter) Reset() {
	imsb.mu.Lock()
	defer imsb.mu.Unlock()
	imsb.ss = nil
	}

ex:

package main

import (
	"context"
	"testing"

	"github.com/stretchr/testify/assert"

	"go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/sdk/trace/tracetest"
)

func TestReset(t *testing.T) {
	sr := tracetest.NewSpanRecorder()
	tp := trace.NewTracerProvider(trace.WithSpanProcessor(sr))
	ctx := context.Background()
	tracer := tp.Tracer("test")

	// test one:
	_, span := tracer.Start(ctx, "one")
	span.End()

	assert.Len(t, sr.Ended(), 1)
	t.Logf("the one span length: %d", len(sr.Ended()))

	// test two:
	sr.Reset() // <=== I don't want the result to be affected by one.
	_, span = tracer.Start(ctx, "two")
	span.End()

	assert.Len(t, sr.Ended(), 1) // <=== I don't want the result to be affected by one.
	t.Logf("the two span length: %d", len(sr.Ended()))
}

output:

=== RUN   TestReset
    main_test.go:24: the one span length: 1
    main_test.go:32: the two span length: 1
--- PASS: TestReset (0.00s)
PASS

Because it is a tool used for testing, I think such operations may occur.

[MrAlias]

Can you provide a use-case for this? All of our uses have not needed this functionality.

[flc1125]

Can you provide a use-case for this? All of our uses have not needed this functionality.

I have already added it to the description.

[dmathieu]

In the same package, InMemoryExporter has a Reset method.
So applying the same to SpanRecorder sounds sensible.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.1%. Comparing base (e98ef1b) to head (1c7d242).
Report is 1 commits behind head on main.

Additional details and impacted files

@@          Coverage Diff          @@
##            main   #5994   +/-   ##
=====================================
  Coverage   82.1%   82.1%           
=====================================
  Files        273     273           
  Lines      23616   23624    +8     
=====================================
+ Hits       19399   19406    +7     
- Misses      3872    3873    +1     
  Partials     345     345           

see 3 files with indirect coverage changes

[pellared]

In the same package, InMemoryExporter has a Reset method. So applying the same to SpanRecorder sounds sensible.

We have also a Reset here: https://pkg.go.dev/go.opentelemetry.io/otel/log/logtest#Recorder.Reset

[pellared]

Can you add a changelog entry?

[MrAlias]

We probably need a Reset method to reuse it for testing.

I'm not in favor of adding additional surface area to the API based on possibilities. I would like there to be valid use cases instead of arguments about symmetry and hypothetical that motivate us.

[flc1125]

We probably need a Reset method to reuse it for testing.

I'm not in favor of adding additional surface area to the API based on possibilities. I would like there to be valid use cases instead of arguments about symmetry and hypothetical that motivate us.

I'll mention the background for adding this feature:

When I was writing the unit tests for otelgin, initially I wanted to implement logic similar to the following:

package main

import (
	"testing"

	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/sdk/trace/tracetest"
)

func TestOtel(t *testing.T) {
	imsb := tracetest.NewInMemoryExporter()
	otel.SetTracerProvider(trace.NewTracerProvider(
		trace.WithSyncer(imsb),
	))

	t.Run("test1", func(t *testing.T) {
		defer imsb.Reset()

		// do something...

		// check imsb...
	})

	t.Run("test2", func(t *testing.T) {
		defer imsb.Reset()

		// do something...

		// check imsb...
	})
}

Because, I hope that imsb and otel.SetTracerProvider only need to be set once for convenience in subsequent repeated use.

However, I found that a large amount of test logic uses SpanRecorder, based on the "herd mentality", I also adjusted to use SpanRecorder. But I discovered that it does not support the Reset() method.

Therefore, this PR came into being.

---

I think the tracetest package should be used for unit testing, since InMemoryExporter has it, and logtest has it too. I feel that SpanExporter could also have it.

[MrAlias]

When I was writing the unit tests for otelgin, initially I wanted to implement logic similar to the following:

package main

import (
	"testing"

	"go.opentelemetry.io/otel"
	"go.opentelemetry.io/otel/sdk/trace"
	"go.opentelemetry.io/otel/sdk/trace/tracetest"
)

func TestOtel(t *testing.T) {
	imsb := tracetest.NewInMemoryExporter()
	otel.SetTracerProvider(trace.NewTracerProvider(
		trace.WithSyncer(imsb),
	))

	t.Run("test1", func(t *testing.T) {
		defer imsb.Reset()

		// do something...

		// check imsb...
	})

	t.Run("test2", func(t *testing.T) {
		defer imsb.Reset()

		// do something...

		// check imsb...
	})
}

Thanks for the use-case.

I would recommend not using the global in testing, but that is a review for another PR. 😉