Skip to content

Commit

Permalink
Encapsulate Record fields
Browse files Browse the repository at this point in the history
  • Loading branch information
pellared committed Dec 5, 2023
1 parent 21bea0f commit c8b3f05
Show file tree
Hide file tree
Showing 6 changed files with 109 additions and 28 deletions.
14 changes: 14 additions & 0 deletions log/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
25 changes: 20 additions & 5 deletions log/benchmark/bench_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand All @@ -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),
Expand Down
7 changes: 5 additions & 2 deletions log/benchmark/logr_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
9 changes: 7 additions & 2 deletions log/benchmark/slog_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
13 changes: 8 additions & 5 deletions log/benchmark/writer_logger_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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))
Expand Down
69 changes: 55 additions & 14 deletions log/record.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit c8b3f05

Please sign in to comment.