From 2d40c6ab9d08baf479b982d0f908d16d1592e9fc Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 4 Oct 2024 10:20:45 +0200 Subject: [PATCH 1/4] chore: avoid doing multiple locks while adding events --- sdk/trace/span.go | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index fc8d10d33b7..b6a80b010d6 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -179,7 +179,8 @@ func (s *recordingSpan) IsRecording() bool { // isRecording returns if this span is being recorded. If this span has ended // this will return false. -// This is done without acquiring a lock. +// +// This method assumes s.mu.Lock is held by the caller. func (s *recordingSpan) isRecording() bool { if s == nil { return false @@ -408,9 +409,10 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) { // the span's duration in case some operation below takes a while. et := monotonicEndTime(s.startTime) - // Do relative expensive check now that we have an end time and see if we - // need to do any more processing. - if !s.IsRecording() { + // Lock the span now that we have an end time and see if we need to do any more processing. + s.mu.Lock() + if !s.isRecording() { + s.mu.Unlock() return } @@ -435,10 +437,11 @@ func (s *recordingSpan) End(options ...trace.SpanEndOption) { } if s.executionTracerTaskEnd != nil { + s.mu.Unlock() s.executionTracerTaskEnd() + s.mu.Lock() } - s.mu.Lock() // Setting endTime to non-zero marks the span as ended and not recording. if config.Timestamp().IsZero() { s.endTime = et @@ -472,7 +475,13 @@ func monotonicEndTime(start time.Time) time.Time { // does not change the Span status. If this span is not being recorded or err is nil // than this method does nothing. func (s *recordingSpan) RecordError(err error, opts ...trace.EventOption) { - if s == nil || err == nil || !s.IsRecording() { + if s == nil || err == nil { + return + } + + s.mu.Lock() + defer s.mu.Unlock() + if !s.isRecording() { return } @@ -508,14 +517,23 @@ func recordStackTrace() string { } // AddEvent adds an event with the provided name and options. If this span is -// not being recorded than this method does nothing. +// not being recorded then this method does nothing. func (s *recordingSpan) AddEvent(name string, o ...trace.EventOption) { - if !s.IsRecording() { + if s == nil { + return + } + + s.mu.Lock() + defer s.mu.Unlock() + if !s.isRecording() { return } s.addEvent(name, o...) } +// addEvent adds an event with the provided name and options. +// +// This method assumes s.mu.Lock is held by the caller. func (s *recordingSpan) addEvent(name string, o ...trace.EventOption) { c := trace.NewEventConfig(o...) e := Event{Name: name, Attributes: c.Attributes(), Time: c.Timestamp()} @@ -532,9 +550,7 @@ func (s *recordingSpan) addEvent(name string, o ...trace.EventOption) { e.Attributes = e.Attributes[:limit] } - s.mu.Lock() s.events.add(e) - s.mu.Unlock() } // SetName sets the name of this span. If this span is not being recorded than From 927914988692459b9da915e5d5c806d54c1f8f24 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Fri, 4 Oct 2024 11:15:53 +0200 Subject: [PATCH 2/4] chore: avoid doing multiple locks while adding links --- sdk/trace/span.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index b6a80b010d6..94b26af0aa3 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -698,7 +698,7 @@ func (s *recordingSpan) Resource() *resource.Resource { } func (s *recordingSpan) AddLink(link trace.Link) { - if !s.IsRecording() { + if s == nil { return } if !link.SpanContext.IsValid() && len(link.Attributes) == 0 && @@ -706,6 +706,12 @@ func (s *recordingSpan) AddLink(link trace.Link) { return } + s.mu.Lock() + defer s.mu.Unlock() + if !s.isRecording() { + return + } + l := Link{SpanContext: link.SpanContext, Attributes: link.Attributes} // Discard attributes over limit. @@ -719,9 +725,7 @@ func (s *recordingSpan) AddLink(link trace.Link) { l.Attributes = l.Attributes[:limit] } - s.mu.Lock() s.links.add(l) - s.mu.Unlock() } // DroppedAttributes returns the number of attributes dropped by the span From e9744b486a8f5a9d1b24064337b24938c3dbcce0 Mon Sep 17 00:00:00 2001 From: warnar boekkooi Date: Mon, 7 Oct 2024 10:58:13 +0200 Subject: [PATCH 3/4] chore: add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f0b871ebc06..5ce5127419c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm - `FilterProcessor.Enabled` in `go.opentelemetry.io/otel/sdk/log/internal/x` now accepts `EnabledParameters` instead of `Record`. (#5791) - The `Record` type in `go.opentelemetry.io/otel/log` is no longer comparable. (#5847) - Performance improvements for the trace SDK `SetAttributes` method in `Span`. (#5864) +- Performance improvements for the trace SDK `AddEvent`, `AddLink`, `RecordError` and `End` methods in `Span`. (#5874) ### Deprecated From 8af1b798c6538f7bce1d76f9a391f9b145cd70d8 Mon Sep 17 00:00:00 2001 From: Warnar Boekkooi Date: Tue, 8 Oct 2024 17:17:13 +0200 Subject: [PATCH 4/4] chore: improve godoc Co-authored-by: David Ashpole --- sdk/trace/span.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/trace/span.go b/sdk/trace/span.go index 94b26af0aa3..730fb85c3ef 100644 --- a/sdk/trace/span.go +++ b/sdk/trace/span.go @@ -531,7 +531,7 @@ func (s *recordingSpan) AddEvent(name string, o ...trace.EventOption) { s.addEvent(name, o...) } -// addEvent adds an event with the provided name and options. +// addEvent adds an event with the provided name and options. // // This method assumes s.mu.Lock is held by the caller. func (s *recordingSpan) addEvent(name string, o ...trace.EventOption) {