From c8b3f05f905be15f69bea97ea7afa1eda245229a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Robert=20Paj=C4=85k?= <pellared@hotmail.com>
Date: Tue, 5 Dec 2023 17:33:58 +0100
Subject: [PATCH] Encapsulate Record fields

---
 log/DESIGN.md                       | 14 ++++++
 log/benchmark/bench_test.go         | 25 ++++++++---
 log/benchmark/logr_test.go          |  7 ++-
 log/benchmark/slog_test.go          |  9 +++-
 log/benchmark/writer_logger_test.go | 13 +++---
 log/record.go                       | 69 +++++++++++++++++++++++------
 6 files changed, 109 insertions(+), 28 deletions(-)

diff --git a/log/DESIGN.md b/log/DESIGN.md
index 25f8d93a702..0ec2073dea9 100644
--- a/log/DESIGN.md
+++ b/log/DESIGN.md
@@ -228,6 +228,20 @@ if it will occur that it could be helpful in some scenarios.
 However, without a strong reason, we prefer to hide the implementation detail
 and have smaller API surface.
 
+### Rejected Alternative: Record with exported fields
+
+There was a proposal that the timestamp,
+observed timestamp, severity number, severity text,
+body properties of the [Emit](https://opentelemetry.io/docs/specs/otel/logs/bridge-api/#emit-a-logrecord)
+are exposed as exported fields of `Record` type.
+This was inspired by [`slog.Record`](https://pkg.go.dev/log/slog#Record)
+design.
+
+We find that exposing all properties via methods is be more consistent.
+Having access to attributes via methods
+and access to other properities via fields
+could be more confusing for the users.
+
 ## Open issues (if applicable)
 
 <!-- A discussion of issues relating to this proposal for which the author does not
diff --git a/log/benchmark/bench_test.go b/log/benchmark/bench_test.go
index 0308b1e400b..e80421a4046 100644
--- a/log/benchmark/bench_test.go
+++ b/log/benchmark/bench_test.go
@@ -58,14 +58,20 @@ func BenchmarkEmit(b *testing.B) {
 				{
 					"no attrs",
 					func() {
-						r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+						r := log.Record{}
+						r.SetTimestamp(testTimestamp)
+						r.SetSeverity(testSeverity)
+						r.SetBody(testBody)
 						tc.logger.Emit(ctx, r)
 					},
 				},
 				{
 					"3 attrs",
 					func() {
-						r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+						r := log.Record{}
+						r.SetTimestamp(testTimestamp)
+						r.SetSeverity(testSeverity)
+						r.SetBody(testBody)
 						r.AddAttributes(
 							attribute.String("string", testString),
 							attribute.Float64("float", testFloat),
@@ -81,7 +87,10 @@ func BenchmarkEmit(b *testing.B) {
 					// should only be from strconv used in writerLogger.
 					"5 attrs",
 					func() {
-						r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+						r := log.Record{}
+						r.SetTimestamp(testTimestamp)
+						r.SetSeverity(testSeverity)
+						r.SetBody(testBody)
 						r.AddAttributes(
 							attribute.String("string", testString),
 							attribute.Float64("float", testFloat),
@@ -95,7 +104,10 @@ func BenchmarkEmit(b *testing.B) {
 				{
 					"10 attrs",
 					func() {
-						r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+						r := log.Record{}
+						r.SetTimestamp(testTimestamp)
+						r.SetSeverity(testSeverity)
+						r.SetBody(testBody)
 						r.AddAttributes(
 							attribute.String("string", testString),
 							attribute.Float64("float", testFloat),
@@ -114,7 +126,10 @@ func BenchmarkEmit(b *testing.B) {
 				{
 					"40 attrs",
 					func() {
-						r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+						r := log.Record{}
+						r.SetTimestamp(testTimestamp)
+						r.SetSeverity(testSeverity)
+						r.SetBody(testBody)
 						r.AddAttributes(
 							attribute.String("string", testString),
 							attribute.Float64("float", testFloat),
diff --git a/log/benchmark/logr_test.go b/log/benchmark/logr_test.go
index b4f637c2bce..16dbc1ebc6e 100644
--- a/log/benchmark/logr_test.go
+++ b/log/benchmark/logr_test.go
@@ -47,9 +47,12 @@ func (s *logrSink) Enabled(level int) bool {
 // Info logs a non-error message with the given key/value pairs as context.
 // It should avoid memory allocations whenever possible.
 func (s *logrSink) Info(level int, msg string, keysAndValues ...any) {
-	lvl := log.Severity(9 - level)
+	record := log.Record{}
+
+	record.SetBody(msg)
 
-	record := log.Record{Severity: lvl, Body: msg}
+	lvl := log.Severity(9 - level)
+	record.SetSeverity(lvl)
 
 	if len(keysAndValues)%2 == 1 {
 		panic("key without a value")
diff --git a/log/benchmark/slog_test.go b/log/benchmark/slog_test.go
index d150b00887a..457efa3eb49 100644
--- a/log/benchmark/slog_test.go
+++ b/log/benchmark/slog_test.go
@@ -38,9 +38,14 @@ type slogHandler struct {
 // Handle handles the Record.
 // It should avoid memory allocations whenever possible.
 func (h *slogHandler) Handle(_ context.Context, r slog.Record) error {
-	lvl := convertLevel(r.Level)
+	record := log.Record{}
+
+	record.SetTimestamp(r.Time)
 
-	record := log.Record{Timestamp: r.Time, Severity: lvl, Body: r.Message}
+	record.SetBody(r.Message)
+
+	lvl := convertLevel(r.Level)
+	record.SetSeverity(lvl)
 
 	r.Attrs(func(a slog.Attr) bool {
 		record.AddAttributes(convertAttr(a))
diff --git a/log/benchmark/writer_logger_test.go b/log/benchmark/writer_logger_test.go
index 52155db82a6..b1043cc25fb 100644
--- a/log/benchmark/writer_logger_test.go
+++ b/log/benchmark/writer_logger_test.go
@@ -22,7 +22,10 @@ func TestWriterLogger(t *testing.T) {
 	sb := &strings.Builder{}
 	l := &writerLogger{w: sb}
 
-	r := log.Record{Timestamp: testTimestamp, Severity: testSeverity, Body: testBody}
+	r := log.Record{}
+	r.SetTimestamp(testTimestamp)
+	r.SetSeverity(testSeverity)
+	r.SetBody(testBody)
 	r.AddAttributes(
 		attribute.String("string", testString),
 		attribute.Float64("float", testFloat),
@@ -44,16 +47,16 @@ type writerLogger struct {
 }
 
 func (l *writerLogger) Emit(_ context.Context, r log.Record) {
-	if !r.Timestamp.IsZero() {
+	if !r.Timestamp().IsZero() {
 		l.write("timestamp=")
-		l.write(strconv.FormatInt(r.Timestamp.Unix(), 10))
+		l.write(strconv.FormatInt(r.Timestamp().Unix(), 10))
 		l.write(" ")
 	}
 	l.write("severity=")
-	l.write(strconv.FormatInt(int64(r.Severity), 10))
+	l.write(strconv.FormatInt(int64(r.Severity()), 10))
 	l.write(" ")
 	l.write("body=")
-	l.write(r.Body)
+	l.write(r.Body())
 	r.WalkAttributes(func(kv attribute.KeyValue) bool {
 		l.write(" ")
 		l.write(string(kv.Key))
diff --git a/log/record.go b/log/record.go
index 4365c16b7fd..5375d18b054 100644
--- a/log/record.go
+++ b/log/record.go
@@ -20,20 +20,11 @@ var errUnsafeAddAttrs = errors.New("unsafely called AddAttrs on copy of Record m
 // Record TODO: comment.
 // TODO: Add unit tests.
 type Record struct {
-	// TODO: comment.
-	Timestamp time.Time
-
-	// TODO: comment.
-	ObservedTimestamp time.Time
-
-	// TODO: comment.
-	Severity Severity
-
-	// TODO: comment.
-	SeverityText string
-
-	// TODO: comment.
-	Body string
+	timestamp         time.Time
+	observedTimestamp time.Time
+	severity          Severity
+	severityText      string
+	body              string
 
 	// The fields below are for optimizing the implementation of
 	// Attributes and AddAttributes.
@@ -87,6 +78,56 @@ const (
 	SeverityFatal4
 )
 
+// Timestamp TODO: comment.
+func (r Record) Timestamp() time.Time {
+	return r.timestamp
+}
+
+// SetTimestamp TODO: comment.
+func (r *Record) SetTimestamp(t time.Time) {
+	r.timestamp = t
+}
+
+// ObservedTimestamp TODO: comment.
+func (r Record) ObservedTimestamp() time.Time {
+	return r.observedTimestamp
+}
+
+// SetObservedTimestamp TODO: comment.
+func (r *Record) SetObservedTimestamp(t time.Time) {
+	r.observedTimestamp = t
+}
+
+// Severity TODO: comment.
+func (r Record) Severity() Severity {
+	return r.severity
+}
+
+// SetSeverity TODO: comment.
+func (r *Record) SetSeverity(s Severity) {
+	r.severity = s
+}
+
+// SeverityText TODO: comment.
+func (r Record) SeverityText() string {
+	return r.severityText
+}
+
+// SetSeverityText TODO: comment.
+func (r *Record) SetSeverityText(s string) {
+	r.severityText = s
+}
+
+// Body TODO: comment.
+func (r Record) Body() string {
+	return r.body
+}
+
+// SetBody TODO: comment.
+func (r *Record) SetBody(s string) {
+	r.body = s
+}
+
 // WalkAttributes calls f on each [attribute.KeyValue] in the [Record].
 // Iteration stops if f returns false.
 func (r Record) WalkAttributes(f func(attribute.KeyValue) bool) {