From d55b152469f69c8482f0fc5b572c86b7c3197820 Mon Sep 17 00:00:00 2001 From: Dmitrii Anoshin Date: Fri, 11 Nov 2022 16:11:37 -0800 Subject: [PATCH] [pdata] Deprecate HexString func on SpanID and TraceID (#6530) `[SpanID|TraceID].HexString` methods currently return an empty string for empty SpanID and TraceID instances. This behavior is not defined in the OTel spec and contradicts with w3c recommendations. We don't want to promote a behavior which possibly can be changed in future. Additionally we introduce `[SpanID|TraceID].String` methods that have the same logic as `HexString` and implement `fmt.Stringer` interface to allow `SpanID` and `TraceID` instances to be used in log statements and avoid unnecessary allocation when log is not emitted. But the `String` method is not encouraged to be used as a replacement for `HexString` because of the concerns as described above. --- .chloggen/deprecate_hexstring.yaml | 4 ++++ .../internal/otlptext/databuffer.go | 6 +++--- exporter/loggingexporter/internal/otlptext/logs.go | 4 ++-- .../loggingexporter/internal/otlptext/traces.go | 6 +++--- exporter/otlphttpexporter/otlp_test.go | 12 ++++++++---- pdata/pcommon/spanid.go | 14 +++++++++++++- pdata/pcommon/spanid_test.go | 10 +++++++++- pdata/pcommon/traceid.go | 14 +++++++++++++- pdata/pcommon/traceid_test.go | 9 ++++++++- 9 files changed, 63 insertions(+), 16 deletions(-) create mode 100755 .chloggen/deprecate_hexstring.yaml diff --git a/.chloggen/deprecate_hexstring.yaml b/.chloggen/deprecate_hexstring.yaml new file mode 100755 index 000000000000..465e779a7f2f --- /dev/null +++ b/.chloggen/deprecate_hexstring.yaml @@ -0,0 +1,4 @@ +change_type: 'deprecation' +component: pdata +note: Deprecate `pcommon.[Span|Trace]ID.HexString` methods. Call `hex.EncodeToString` explicitly instead. +issues: [6514] diff --git a/exporter/loggingexporter/internal/otlptext/databuffer.go b/exporter/loggingexporter/internal/otlptext/databuffer.go index 03bbbdfacafe..117392b26578 100644 --- a/exporter/loggingexporter/internal/otlptext/databuffer.go +++ b/exporter/loggingexporter/internal/otlptext/databuffer.go @@ -34,7 +34,7 @@ func (b *dataBuffer) logEntry(format string, a ...interface{}) { b.buf.WriteString("\n") } -func (b *dataBuffer) logAttr(attr string, value string) { +func (b *dataBuffer) logAttr(attr string, value interface{}) { b.logEntry(" %-15s: %s", attr, value) } @@ -258,8 +258,8 @@ func (b *dataBuffer) logLinks(description string, sl ptrace.SpanLinkSlice) { for i := 0; i < sl.Len(); i++ { l := sl.At(i) b.logEntry("SpanLink #%d", i) - b.logEntry(" -> Trace ID: %s", l.TraceID().HexString()) - b.logEntry(" -> ID: %s", l.SpanID().HexString()) + b.logEntry(" -> Trace ID: %s", l.TraceID()) + b.logEntry(" -> ID: %s", l.SpanID()) b.logEntry(" -> TraceState: %s", l.TraceState().AsRaw()) b.logEntry(" -> DroppedAttributesCount: %d", l.DroppedAttributesCount()) if l.Attributes().Len() == 0 { diff --git a/exporter/loggingexporter/internal/otlptext/logs.go b/exporter/loggingexporter/internal/otlptext/logs.go index ec3eb5f6b015..3d4a963b198c 100644 --- a/exporter/loggingexporter/internal/otlptext/logs.go +++ b/exporter/loggingexporter/internal/otlptext/logs.go @@ -51,8 +51,8 @@ func (textLogsMarshaler) MarshalLogs(ld plog.Logs) ([]byte, error) { buf.logEntry("SeverityNumber: %s(%d)", lr.SeverityNumber(), lr.SeverityNumber()) buf.logEntry("Body: %s", valueToString(lr.Body())) buf.logAttributes("Attributes", lr.Attributes()) - buf.logEntry("Trace ID: %s", lr.TraceID().HexString()) - buf.logEntry("Span ID: %s", lr.SpanID().HexString()) + buf.logEntry("Trace ID: %s", lr.TraceID()) + buf.logEntry("Span ID: %s", lr.SpanID()) buf.logEntry("Flags: %d", lr.Flags()) } } diff --git a/exporter/loggingexporter/internal/otlptext/traces.go b/exporter/loggingexporter/internal/otlptext/traces.go index ee7b8a7a3b22..20e9c02d88bc 100644 --- a/exporter/loggingexporter/internal/otlptext/traces.go +++ b/exporter/loggingexporter/internal/otlptext/traces.go @@ -45,9 +45,9 @@ func (textTracesMarshaler) MarshalTraces(td ptrace.Traces) ([]byte, error) { for k := 0; k < spans.Len(); k++ { buf.logEntry("Span #%d", k) span := spans.At(k) - buf.logAttr("Trace ID", span.TraceID().HexString()) - buf.logAttr("Parent ID", span.ParentSpanID().HexString()) - buf.logAttr("ID", span.SpanID().HexString()) + buf.logAttr("Trace ID", span.TraceID()) + buf.logAttr("Parent ID", span.ParentSpanID()) + buf.logAttr("ID", span.SpanID()) buf.logAttr("Name", span.Name()) buf.logAttr("Kind", span.Kind().String()) buf.logAttr("Start time", span.StartTimestamp().String()) diff --git a/exporter/otlphttpexporter/otlp_test.go b/exporter/otlphttpexporter/otlp_test.go index d43f26345af1..83361af35f2c 100644 --- a/exporter/otlphttpexporter/otlp_test.go +++ b/exporter/otlphttpexporter/otlp_test.go @@ -261,8 +261,10 @@ func TestIssue_4221(t *testing.T) { tr := ptraceotlp.NewExportRequest() require.NoError(t, tr.UnmarshalProto(unbase64Data)) span := tr.Traces().ResourceSpans().At(0).ScopeSpans().At(0).Spans().At(0) - assert.Equal(t, "4303853f086f4f8c86cf198b6551df84", span.TraceID().HexString()) - assert.Equal(t, "e5513c32795c41b9", span.SpanID().HexString()) + traceID := span.TraceID() + assert.Equal(t, "4303853f086f4f8c86cf198b6551df84", hex.EncodeToString(traceID[:])) + spanID := span.SpanID() + assert.Equal(t, "e5513c32795c41b9", hex.EncodeToString(spanID[:])) })) exp := startTracesExporter(t, "", svr.URL) @@ -281,14 +283,16 @@ func TestIssue_4221(t *testing.T) { require.NoError(t, err) copy(traceIDBytes[:], traceIDBytesSlice) span.SetTraceID(traceIDBytes) - assert.Equal(t, "4303853f086f4f8c86cf198b6551df84", span.TraceID().HexString()) + traceID := span.TraceID() + assert.Equal(t, "4303853f086f4f8c86cf198b6551df84", hex.EncodeToString(traceID[:])) var spanIDBytes [8]byte spanIDBytesSlice, err := hex.DecodeString("e5513c32795c41b9") require.NoError(t, err) copy(spanIDBytes[:], spanIDBytesSlice) span.SetSpanID(spanIDBytes) - assert.Equal(t, "e5513c32795c41b9", span.SpanID().HexString()) + spanID := span.SpanID() + assert.Equal(t, "e5513c32795c41b9", hex.EncodeToString(spanID[:])) span.SetEndTimestamp(1634684637873000000) span.Attributes().PutInt("span_index", 3) diff --git a/pdata/pcommon/spanid.go b/pdata/pcommon/spanid.go index 44c4f249fae3..e51c115d9dc5 100644 --- a/pdata/pcommon/spanid.go +++ b/pdata/pcommon/spanid.go @@ -29,7 +29,19 @@ func NewSpanIDEmpty() SpanID { return emptySpanID } -// HexString returns hex representation of the SpanID. +// String returns string representation of the SpanID. +// +// Important: Don't rely on this method to get a string identifier of SpanID, +// Use hex.EncodeToString explicitly instead. +// This method meant to implement Stringer interface for display purposes only. +func (ms SpanID) String() string { + if ms.IsEmpty() { + return "" + } + return hex.EncodeToString(ms[:]) +} + +// Deprecated: [0.65.0] Call hex.EncodeToString explicitly instead. func (ms SpanID) HexString() string { if ms.IsEmpty() { return "" diff --git a/pdata/pcommon/spanid_test.go b/pdata/pcommon/spanid_test.go index c8ce41321953..d92327dc6d64 100644 --- a/pdata/pcommon/spanid_test.go +++ b/pdata/pcommon/spanid_test.go @@ -20,7 +20,7 @@ import ( "github.com/stretchr/testify/assert" ) -func TestNewSpanID(t *testing.T) { +func TestSpanID(t *testing.T) { sid := SpanID([8]byte{1, 2, 3, 4, 4, 3, 2, 1}) assert.Equal(t, [8]byte{1, 2, 3, 4, 4, 3, 2, 1}, [8]byte(sid)) assert.False(t, sid.IsEmpty()) @@ -40,6 +40,14 @@ func TestSpanIDHexString(t *testing.T) { assert.Equal(t, "1223ad1223ad1223", sid.HexString()) } +func TestSpanIDString(t *testing.T) { + sid := SpanID([8]byte{}) + assert.Equal(t, "", sid.String()) + + sid = SpanID([8]byte{0x12, 0x23, 0xAD, 0x12, 0x23, 0xAD, 0x12, 0x23}) + assert.Equal(t, "1223ad1223ad1223", sid.String()) +} + func TestSpanIDImmutable(t *testing.T) { initialBytes := [8]byte{0x12, 0x23, 0xAD, 0x12, 0x23, 0xAD, 0x12, 0x23} sid := SpanID(initialBytes) diff --git a/pdata/pcommon/traceid.go b/pdata/pcommon/traceid.go index ed47d67a6d54..5a035e05c6bc 100644 --- a/pdata/pcommon/traceid.go +++ b/pdata/pcommon/traceid.go @@ -30,7 +30,19 @@ func NewTraceIDEmpty() TraceID { return emptyTraceID } -// HexString returns hex representation of the TraceID. +// String returns string representation of the TraceID. +// +// Important: Don't rely on this method to get a string identifier of TraceID. +// Use hex.EncodeToString explicitly instead. +// This method meant to implement Stringer interface for display purposes only. +func (ms TraceID) String() string { + if ms.IsEmpty() { + return "" + } + return hex.EncodeToString(ms[:]) +} + +// Deprecated: [0.65.0] Call hex.EncodeToString explicitly instead. func (ms TraceID) HexString() string { if ms.IsEmpty() { return "" diff --git a/pdata/pcommon/traceid_test.go b/pdata/pcommon/traceid_test.go index 9aaa476daa0d..257298711a17 100644 --- a/pdata/pcommon/traceid_test.go +++ b/pdata/pcommon/traceid_test.go @@ -24,7 +24,6 @@ func TestTraceID(t *testing.T) { tid := TraceID([16]byte{1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}) assert.Equal(t, [16]byte{1, 2, 3, 4, 5, 6, 7, 8, 8, 7, 6, 5, 4, 3, 2, 1}, [16]byte(tid)) assert.False(t, tid.IsEmpty()) - assert.Equal(t, "01020304050607080807060504030201", tid.HexString()) } func TestNewTraceIDEmpty(t *testing.T) { @@ -41,6 +40,14 @@ func TestTraceIDHexString(t *testing.T) { assert.Equal(t, "12345678123456781234567812345678", tid.HexString()) } +func TestTraceIDString(t *testing.T) { + tid := TraceID([16]byte{}) + assert.Equal(t, "", tid.String()) + + tid = [16]byte{0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78} + assert.Equal(t, "12345678123456781234567812345678", tid.String()) +} + func TestTraceIDImmutable(t *testing.T) { initialBytes := [16]byte{0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78, 0x12, 0x34, 0x56, 0x78} tid := TraceID(initialBytes)