From fd256155608cffda78efca681f638c36b078bc2f Mon Sep 17 00:00:00 2001
From: irfan sharif <irfanmahmoudsharif@gmail.com>
Date: Fri, 5 Feb 2021 17:36:38 -0500
Subject: [PATCH] tracing: introduce a per-span limit for structured events

Release note: None
---
 pkg/util/ring/ring_buffer.go  |  2 +-
 pkg/util/tracing/BUILD.bazel  |  1 +
 pkg/util/tracing/crdbspan.go  | 18 ++++++++++++------
 pkg/util/tracing/span_test.go | 28 ++++++++++++++++++++++++++++
 pkg/util/tracing/tracer.go    |  7 ++++++-
 5 files changed, 48 insertions(+), 8 deletions(-)

diff --git a/pkg/util/ring/ring_buffer.go b/pkg/util/ring/ring_buffer.go
index d0cf5120e0c5..e1e22ca5c41a 100644
--- a/pkg/util/ring/ring_buffer.go
+++ b/pkg/util/ring/ring_buffer.go
@@ -134,7 +134,7 @@ func (r *Buffer) RemoveLast() {
 	}
 }
 
-// Reserve reserves the provided number of elemnets in the Buffer. It is an
+// Reserve reserves the provided number of elements in the Buffer. It is an
 // error to reserve a size less than the Buffer's current length.
 func (r *Buffer) Reserve(n int) {
 	if n < r.Len() {
diff --git a/pkg/util/tracing/BUILD.bazel b/pkg/util/tracing/BUILD.bazel
index 0491b6ce17bf..7ccf18da8cb9 100644
--- a/pkg/util/tracing/BUILD.bazel
+++ b/pkg/util/tracing/BUILD.bazel
@@ -24,6 +24,7 @@ go_library(
         "//pkg/util/envutil",
         "//pkg/util/iterutil",
         "//pkg/util/protoutil",
+        "//pkg/util/ring",
         "//pkg/util/syncutil",
         "//pkg/util/timeutil",
         "//pkg/util/tracing/tracingpb",
diff --git a/pkg/util/tracing/crdbspan.go b/pkg/util/tracing/crdbspan.go
index 113fcd090c09..fc67ae677add 100644
--- a/pkg/util/tracing/crdbspan.go
+++ b/pkg/util/tracing/crdbspan.go
@@ -17,6 +17,7 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/util/ring"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
@@ -79,7 +80,7 @@ type crdbSpanMu struct {
 	tags opentracing.Tags
 
 	stats      SpanStats
-	structured []Structured
+	structured ring.Buffer // of Structured events
 
 	// The Span's associated baggage.
 	baggage map[string]string
@@ -224,7 +225,11 @@ func (s *crdbSpan) record(msg string) {
 func (s *crdbSpan) recordStructured(item Structured) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
-	s.mu.structured = append(s.mu.structured, item)
+
+	if s.mu.structured.Len() == maxStructuredEventsPerSpan {
+		s.mu.structured.RemoveLast()
+	}
+	s.mu.structured.AddFirst(item)
 }
 
 func (s *crdbSpan) setBaggageItemAndTag(restrictedKey, value string) {
@@ -300,10 +305,11 @@ func (s *crdbSpan) getRecordingLocked(m mode) tracingpb.RecordedSpan {
 		rs.DeprecatedStats = stats
 	}
 
-	if s.mu.structured != nil {
-		rs.InternalStructured = make([]*types.Any, 0, len(s.mu.structured))
-		for i := range s.mu.structured {
-			item, err := types.MarshalAny(s.mu.structured[i])
+	if numEvents := s.mu.structured.Len(); numEvents != 0 {
+		rs.InternalStructured = make([]*types.Any, 0, numEvents)
+		for i := 0; i < numEvents; i++ {
+			event := s.mu.structured.Get(i).(Structured)
+			item, err := types.MarshalAny(event)
 			if err != nil {
 				// An error here is an error from Marshal; these
 				// are unlikely to happen.
diff --git a/pkg/util/tracing/span_test.go b/pkg/util/tracing/span_test.go
index c9df9085d3d0..ac98e2bef233 100644
--- a/pkg/util/tracing/span_test.go
+++ b/pkg/util/tracing/span_test.go
@@ -213,6 +213,34 @@ func TestSpanRecordStructured(t *testing.T) {
 	`))
 }
 
+func TestSpanRecordStructuredLimit(t *testing.T) {
+	tr := NewTracer()
+	tr._mode = int32(modeBackground)
+	sp := tr.StartSpan("root", WithForceRealSpan())
+	defer sp.Finish()
+
+	const extra = 10
+	for i := int32(1); i <= maxStructuredEventsPerSpan+extra; i++ {
+		sp.RecordStructured(&types.Int32Value{Value: i})
+	}
+	rec := sp.GetRecording()
+	require.Len(t, rec, 1)
+	require.Len(t, rec[0].InternalStructured, maxStructuredEventsPerSpan)
+
+	first := rec[0].InternalStructured[0]
+	last := rec[0].InternalStructured[len(rec[0].InternalStructured)-1]
+	var d1 types.DynamicAny
+	require.NoError(t, types.UnmarshalAny(first, &d1))
+	require.IsType(t, (*types.Int32Value)(nil), d1.Message)
+
+	var res int32
+	require.NoError(t, types.StdInt32Unmarshal(&res, first.Value))
+	require.Equal(t, res, int32(maxStructuredEventsPerSpan+extra))
+
+	require.NoError(t, types.StdInt32Unmarshal(&res, last.Value))
+	require.Equal(t, res, int32(extra+1))
+}
+
 func TestNonVerboseChildSpanRegisteredWithParent(t *testing.T) {
 	tr := NewTracer()
 	tr._mode = int32(modeBackground)
diff --git a/pkg/util/tracing/tracer.go b/pkg/util/tracing/tracer.go
index bbba9588b9a7..3f4f247963ad 100644
--- a/pkg/util/tracing/tracer.go
+++ b/pkg/util/tracing/tracer.go
@@ -41,8 +41,12 @@ import (
 const verboseTracingBaggageKey = "sb"
 
 const (
-	// maxLogsPerSpan limits the number of logs in a Span; use a comfortable limit.
+	// maxLogsPerSpan limits the number of logs in a Span; use a comfortable
+	// limit.
 	maxLogsPerSpan = 1000
+	// maxStructuredEventsPerSpan limits the number of structured events in a
+	// span; use a comfortable limit.
+	maxStructuredEventsPerSpan = 50
 	// maxChildrenPerSpan limits the number of (direct) child spans in a Span.
 	maxChildrenPerSpan = 1000
 )
@@ -399,6 +403,7 @@ func (t *Tracer) startSpanGeneric(
 			duration: -1, // unfinished
 		},
 	}
+	helper.crdbSpan.mu.structured.Reserve(maxStructuredEventsPerSpan)
 	helper.span.i = spanInner{
 		tracer: t,
 		crdb:   &helper.crdbSpan,