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

sdk/log: SimpleProcessor to not panic for zero value #5665

Merged
merged 3 commits into from
Aug 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
This module is unstable and breaking changes may be introduced.
See our [versioning policy](VERSIONING.md) for more information about these stability guarantees. (#5629)
- Add `InstrumentationScope` field to `SpanStub` in `go.opentelemetry.io/otel/sdk/trace/tracetest`, as a replacement for the deprecated `InstrumentationLibrary`. (#5627)
- Zero value of `SimpleProcessor` in `go.opentelemetry.io/otel/sdk/log` no longer panics. (#5665)

### Changed

- `Processor.OnEmit` in `go.opentelemetry.io/otel/sdk/log` now accepts a pointer to `Record` instead of a value so that the record modifications done in a processor are propagated to subsequent registered processors. (#5636)
- `SimpleProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log` now returns `false` if the exporter is `nil`. (#5665)

### Fixed

Expand Down
22 changes: 16 additions & 6 deletions sdk/log/simple.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ import (
var _ Processor = (*SimpleProcessor)(nil)

// SimpleProcessor is an processor that synchronously exports log records.
//
// Use [NewSimpleProcessor] to create a SimpleProcessor.
type SimpleProcessor struct {
exporter Exporter
}
Expand All @@ -25,10 +27,6 @@ type SimpleProcessor struct {
// [NewBatchProcessor] instead. However, there may be exceptions where certain
// [Exporter] implementations perform better with this Processor.
func NewSimpleProcessor(exporter Exporter, _ ...SimpleProcessorOption) *SimpleProcessor {
if exporter == nil {
// Do not panic on nil exporter.
exporter = defaultNoopExporter
}
return &SimpleProcessor{exporter: exporter}
}

Expand All @@ -41,6 +39,10 @@ var simpleProcRecordsPool = sync.Pool{

// OnEmit batches provided log record.
func (s *SimpleProcessor) OnEmit(ctx context.Context, r *Record) error {
if s.exporter == nil {
return nil
}

records := simpleProcRecordsPool.Get().(*[]Record)
(*records)[0] = *r
defer func() {
Expand All @@ -50,18 +52,26 @@ func (s *SimpleProcessor) OnEmit(ctx context.Context, r *Record) error {
return s.exporter.Export(ctx, *records)
}

// Enabled returns true.
// Enabled returns true if the exporter is not nil.
func (s *SimpleProcessor) Enabled(context.Context, Record) bool {
return true
return s.exporter != nil
}

// Shutdown shuts down the expoter.
func (s *SimpleProcessor) Shutdown(ctx context.Context) error {
if s.exporter == nil {
return nil
}

return s.exporter.Shutdown(ctx)
}

// ForceFlush flushes the exporter.
func (s *SimpleProcessor) ForceFlush(ctx context.Context) error {
if s.exporter == nil {
return nil
}

return s.exporter.ForceFlush(ctx)
}

Expand Down
15 changes: 14 additions & 1 deletion sdk/log/simple_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ func TestSimpleProcessorOnEmit(t *testing.T) {
}

func TestSimpleProcessorEnabled(t *testing.T) {
s := log.NewSimpleProcessor(nil)
e := new(exporter)
s := log.NewSimpleProcessor(e)
assert.True(t, s.Enabled(context.Background(), log.Record{}))
}

Expand All @@ -69,6 +70,18 @@ func TestSimpleProcessorForceFlush(t *testing.T) {
require.True(t, e.forceFlushCalled, "exporter ForceFlush not called")
}

func TestSimpleProcessorEmpty(t *testing.T) {
assert.NotPanics(t, func() {
var s log.SimpleProcessor
ctx := context.Background()
record := new(log.Record)
assert.NoError(t, s.OnEmit(ctx, record), "OnEmit")
assert.False(t, s.Enabled(ctx, *record), "Enabled")
assert.NoError(t, s.ForceFlush(ctx), "ForceFlush")
assert.NoError(t, s.Shutdown(ctx), "Shutdown")
})
}

func TestSimpleProcessorConcurrentSafe(t *testing.T) {
const goRoutineN = 10

Expand Down