Skip to content

Commit

Permalink
util/tracing: remove DeprecatedInternalStructured
Browse files Browse the repository at this point in the history
This recording field was necessary for compatibility with 21.1. Now that
21.2 is out, we no longer need it, and it costs allocations.

Release note: None
  • Loading branch information
andreimatei committed Oct 4, 2021
1 parent 7d752a1 commit 0e20ab8
Show file tree
Hide file tree
Showing 9 changed files with 69 additions and 171 deletions.
1 change: 0 additions & 1 deletion pkg/server/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,6 @@ go_test(
"@com_github_dustin_go_humanize//:go-humanize",
"@com_github_gogo_protobuf//jsonpb",
"@com_github_gogo_protobuf//proto",
"@com_github_gogo_protobuf//types",
"@com_github_grpc_ecosystem_grpc_gateway//runtime:go_default_library",
"@com_github_kr_pretty//:pretty",
"@com_github_lib_pq//:pq",
Expand Down
28 changes: 13 additions & 15 deletions pkg/server/node_tenant_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/util/tracing"
"github.com/cockroachdb/cockroach/pkg/util/tracing/tracingpb"
"github.com/cockroachdb/logtags"
ptypes "github.com/gogo/protobuf/types"
"github.com/stretchr/testify/require"
"go.opentelemetry.io/otel/attribute"
)
Expand Down Expand Up @@ -95,20 +94,19 @@ func TestRedactRecordingForTenant(t *testing.T) {
// that may leak from the KV layer to tenants. If it does, update
// redactRecordingForTenant appropriately.
type calcifiedRecordedSpan struct {
TraceID uint64
SpanID uint64
ParentSpanID uint64
Operation string
Baggage map[string]string
Tags map[string]string
StartTime time.Time
Duration time.Duration
RedactableLogs bool
Logs []tracingpb.LogRecord
DeprecatedInternalStructured []*ptypes.Any
GoroutineID uint64
Finished bool
StructuredRecords []tracingpb.StructuredRecord
TraceID uint64
SpanID uint64
ParentSpanID uint64
Operation string
Baggage map[string]string
Tags map[string]string
StartTime time.Time
Duration time.Duration
RedactableLogs bool
Logs []tracingpb.LogRecord
GoroutineID uint64
Finished bool
StructuredRecords []tracingpb.StructuredRecord
}
_ = (*calcifiedRecordedSpan)((*tracingpb.RecordedSpan)(nil))
})
Expand Down
5 changes: 2 additions & 3 deletions pkg/sql/protoreflect/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,8 @@ func TestMessageToJSONBRoundTrip(t *testing.T) {
// nested inside other message; with maps
pbname: "cockroach.util.tracing.tracingpb.RecordedSpan",
message: &tracingpb.RecordedSpan{
TraceID: 123,
Tags: map[string]string{"one": "1", "two": "2", "three": "3"},
DeprecatedInternalStructured: []*pbtypes.Any{makeAny(t, &descpb.ColumnDescriptor{Name: "bogus stats"})},
TraceID: 123,
Tags: map[string]string{"one": "1", "two": "2", "three": "3"},
StructuredRecords: []tracingpb.StructuredRecord{{
Time: timeutil.Now(),
Payload: makeAny(t, &descpb.ColumnDescriptor{Name: "bogus stats"})}},
Expand Down
6 changes: 0 additions & 6 deletions pkg/util/tracing/crdbspan.go
Original file line number Diff line number Diff line change
Expand Up @@ -434,16 +434,10 @@ func (s *crdbSpan) getRecordingLocked(wantTags bool) tracingpb.RecordedSpan {
}

if numEvents := s.mu.recording.structured.Len(); numEvents != 0 {
// TODO(adityamaru): Stop writing to DeprecatedInternalStructured in 22.1.
rs.DeprecatedInternalStructured = make([]*types.Any, numEvents)
rs.StructuredRecords = make([]tracingpb.StructuredRecord, numEvents)
for i := 0; i < numEvents; i++ {
event := s.mu.recording.structured.Get(i).(*tracingpb.StructuredRecord)
rs.StructuredRecords[i] = *event
// Write the Structured payload stored in the StructuredRecord, since
// nodes older than 21.2 expect a Structured event when they fetch
// recordings.
rs.DeprecatedInternalStructured[i] = event.Payload
}
}

Expand Down
4 changes: 0 additions & 4 deletions pkg/util/tracing/grpc_interceptor_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,16 +209,13 @@ func TestGRPCInterceptors(t *testing.T) {
require.NoError(t, err)
var rec tracingpb.RecordedSpan
require.NoError(t, types.UnmarshalAny(recAny, &rec))
require.Len(t, rec.DeprecatedInternalStructured, 1)
require.Len(t, rec.StructuredRecords, 1)
sp.ImportRemoteSpans([]tracingpb.RecordedSpan{rec})
sp.Finish()
var deprecatedN int
var n int
finalRecs := sp.GetRecording()
sp.SetVerbose(false)
for _, rec := range finalRecs {
deprecatedN += len(rec.DeprecatedInternalStructured)
n += len(rec.StructuredRecords)
// Remove all of the _unfinished tags. These crop up because
// in this test we are pulling the recorder in the handler impl,
Expand All @@ -230,7 +227,6 @@ func TestGRPCInterceptors(t *testing.T) {
delete(rec.Tags, "_unfinished")
delete(rec.Tags, "_verbose")
}
require.Equal(t, 1, deprecatedN)
require.Equal(t, 1, n)

exp := fmt.Sprintf(`
Expand Down
7 changes: 0 additions & 7 deletions pkg/util/tracing/span_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,6 @@ func TestSpanRecordStructured(t *testing.T) {
sp.RecordStructured(&types.Int32Value{Value: 4})
rec := sp.GetRecording()
require.Len(t, rec, 1)
require.Len(t, rec[0].DeprecatedInternalStructured, 1)
deprecatedItem := rec[0].DeprecatedInternalStructured[0]
var deprecatedStructured types.DynamicAny
require.NoError(t, types.UnmarshalAny(deprecatedItem, &deprecatedStructured))
require.IsType(t, (*types.Int32Value)(nil), deprecatedStructured.Message)

require.Len(t, rec[0].StructuredRecords, 1)
item := rec[0].StructuredRecords[0]
var d1 types.DynamicAny
Expand Down Expand Up @@ -416,7 +410,6 @@ func TestNonVerboseChildSpanRegisteredWithParent(t *testing.T) {
// Check that the child span (incl its payload) is in the recording.
rec := sp.GetRecording()
require.Len(t, rec, 2)
require.Len(t, rec[1].DeprecatedInternalStructured, 1)
require.Len(t, rec[1].StructuredRecords, 1)
}

Expand Down
10 changes: 0 additions & 10 deletions pkg/util/tracing/tracingpb/recorded_span.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,6 @@ func (s *RecordedSpan) String() string {
// Structured visits the data passed to RecordStructured for the Span from which
// the RecordedSpan was created.
func (s *RecordedSpan) Structured(visit func(*types.Any, time.Time)) {
// Check if the RecordedSpan is from a node running a version less than 21.2.
// If it is, we set the "recorded at" time to the StartTime of the span.
// TODO(adityamaru): Remove this code in 22.1 since all RecordedSpans will
// have StructuredRecords in 21.2+ nodes.
if s.StructuredRecords == nil {
for _, item := range s.DeprecatedInternalStructured {
visit(item, s.StartTime)
}
return
}
for _, sr := range s.StructuredRecords {
visit(sr.Payload, sr.Time)
}
Expand Down
169 changes: 53 additions & 116 deletions pkg/util/tracing/tracingpb/recorded_span.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 0e20ab8

Please sign in to comment.