From 9d90c237bff71f3d6b7153839caa7aeb075ed7b2 Mon Sep 17 00:00:00 2001
From: Prashant V <prashant@prashantv.com>
Date: Fri, 8 Sep 2023 18:01:58 -0700
Subject: [PATCH] zapslog: Handle empty attrs centrally

Follow-up to #1344 to handle empty attributes using a skip field
in a single place, so each caller of `convertAttrToField` doesn't
have to check for empty attributes explicitly.
---
 exp/zapslog/handler.go      | 12 +++++-------
 exp/zapslog/handler_test.go | 12 ++++++++++++
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/exp/zapslog/handler.go b/exp/zapslog/handler.go
index 6791a262c..982d9bccd 100644
--- a/exp/zapslog/handler.go
+++ b/exp/zapslog/handler.go
@@ -67,6 +67,11 @@ func (gs groupObject) MarshalLogObject(enc zapcore.ObjectEncoder) error {
 }
 
 func convertAttrToField(attr slog.Attr) zapcore.Field {
+	if attr.Equal(slog.Attr{}) {
+		// Ignore empty attrs.
+		return zap.Skip()
+	}
+
 	switch attr.Value.Kind() {
 	case slog.KindBool:
 		return zap.Bool(attr.Key, attr.Value.Bool())
@@ -154,10 +159,6 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 
 	fields := make([]zapcore.Field, 0, record.NumAttrs())
 	record.Attrs(func(attr slog.Attr) bool {
-		if attr.Equal(slog.Attr{}) {
-			return true // ignore empty attributes
-		}
-
 		fields = append(fields, convertAttrToField(attr))
 		return true
 	})
@@ -170,9 +171,6 @@ func (h *Handler) Handle(ctx context.Context, record slog.Record) error {
 func (h *Handler) WithAttrs(attrs []slog.Attr) slog.Handler {
 	fields := make([]zapcore.Field, 0, len(attrs))
 	for _, attr := range attrs {
-		if attr.Equal(slog.Attr{}) {
-			continue // ignore empty attributes
-		}
 		fields = append(fields, convertAttrToField(attr))
 	}
 	return h.withFields(fields...)
diff --git a/exp/zapslog/handler_test.go b/exp/zapslog/handler_test.go
index a78f394bd..4cd4b2747 100644
--- a/exp/zapslog/handler_test.go
+++ b/exp/zapslog/handler_test.go
@@ -113,6 +113,18 @@ func TestEmptyAttr(t *testing.T) {
 			"foo": "bar",
 		}, logs[0].ContextMap(), "Unexpected context")
 	})
+
+	t.Run("Group", func(t *testing.T) {
+		sl.With("k", slog.GroupValue(slog.String("foo", "bar"), slog.Attr{})).Info("msg")
+
+		logs := observedLogs.TakeAll()
+		require.Len(t, logs, 1, "Expected exactly one entry to be logged")
+		assert.Equal(t, map[string]any{
+			"k": map[string]any{
+				"foo": "bar",
+			},
+		}, logs[0].ContextMap(), "Unexpected context")
+	})
 }
 
 func TestWithName(t *testing.T) {