feat!: simplify and improve logging hook (#372)

* refactor: fix logging hook output

Signed-off-by: Sahid Velji <[email protected]>

* fix: make consts unexported and camelCase

Signed-off-by: Sahid Velji <[email protected]>

* refactor: remove `NewCustomLoggingHook` in favor of `NewLoggingHook`

Signed-off-by: Sahid Velji <[email protected]>

* fixup: func name

Signed-off-by: Todd Baert <[email protected]>

---------

Signed-off-by: Sahid Velji <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
Co-authored-by: Todd Baert <[email protected]>

Author: sahidvelji

openfeature/hooks/logging_hook.go

@@ -8,16 +8,18 @@ import (
 )
 
 const (
-	DOMAIN_KEY             = "domain"
-	PROVIDER_NAME_KEY      = "provider_name"
-	FLAG_KEY_KEY           = "flag_key"
-	DEFAULT_VALUE_KEY      = "default_value"
-	EVALUATION_CONTEXT_KEY = "evaluation_context"
-	ERROR_MESSAGE_KEY      = "error_message"
-	REASON_KEY             = "reason"
-	VARIANT_KEY            = "variant"
-	VALUE_KEY              = "value"
-	STAGE_KEY              = "stage"
+	domainKey            = "domain"
+	providerNameKey      = "provider_name"
+	flagKeyKey           = "flag_key"
+	defaultValueKey      = "default_value"
+	evaluationContextKey = "evaluation_context"
+	targetingKeyKey      = "targeting_key"
+	attributesKey        = "attributes"
+	errorMessageKey      = "error_message"
+	reasonKey            = "reason"
+	variantKey           = "variant"
+	valueKey             = "value"
+	stageKey             = "stage"
 )
 
 // LoggingHook is a [of.Hook] that logs the flag evaluation lifecycle.
@@ -26,46 +28,35 @@ type LoggingHook struct {
 	logger                   *slog.Logger
 }
 
-// NewLoggingHook returns a new [LoggingHook] with the default logger.
-// To provide a custom logger, use [NewCustomLoggingHook].
-func NewLoggingHook(includeEvaluationContext bool) (*LoggingHook, error) {
-	return NewCustomLoggingHook(includeEvaluationContext, slog.Default())
-}
-
-// NewCustomLoggingHook returns a new [LoggingHook] with the provided logger.
-func NewCustomLoggingHook(includeEvaluationContext bool, logger *slog.Logger) (*LoggingHook, error) {
+// NewLoggingHook returns a new [LoggingHook] with the provided logger.
+func NewLoggingHook(includeEvaluationContext bool, logger *slog.Logger) *LoggingHook {
 	return &LoggingHook{
 		logger:                   logger,
 		includeEvaluationContext: includeEvaluationContext,
-	}, nil
-}
-
-type MarshaledEvaluationContext struct {
-	TargetingKey string
-	Attributes   map[string]any
+	}
 }
 
 func (h *LoggingHook) buildArgs(hookContext of.HookContext) []slog.Attr {
 	args := []slog.Attr{
-		slog.String(DOMAIN_KEY, hookContext.ClientMetadata().Domain()),
-		slog.String(PROVIDER_NAME_KEY, hookContext.ProviderMetadata().Name),
-		slog.String(FLAG_KEY_KEY, hookContext.FlagKey()),
-		slog.Any(DEFAULT_VALUE_KEY, hookContext.DefaultValue()),
+		slog.String(domainKey, hookContext.ClientMetadata().Domain()),
+		slog.String(providerNameKey, hookContext.ProviderMetadata().Name),
+		slog.String(flagKeyKey, hookContext.FlagKey()),
+		slog.Any(defaultValueKey, hookContext.DefaultValue()),
 	}
 	if h.includeEvaluationContext {
-		marshaledEvaluationContext := MarshaledEvaluationContext{
-			TargetingKey: hookContext.EvaluationContext().TargetingKey(),
-			Attributes:   hookContext.EvaluationContext().Attributes(),
-		}
-		args = append(args, slog.Any(EVALUATION_CONTEXT_KEY, marshaledEvaluationContext))
+		args = append(args,
+			slog.Group(evaluationContextKey,
+				slog.String(targetingKeyKey, hookContext.EvaluationContext().TargetingKey()),
+				slog.Any(attributesKey, hookContext.EvaluationContext().Attributes()),
+			))
 	}
 
 	return args
 }
 
 func (h *LoggingHook) Before(ctx context.Context, hookContext of.HookContext, hookHints of.HookHints) (*of.EvaluationContext, error) {
 	args := h.buildArgs(hookContext)
-	args = append(args, slog.String(STAGE_KEY, "before"))
+	args = append(args, slog.String(stageKey, "before"))
 	h.logger.LogAttrs(ctx, slog.LevelDebug, "Before stage", args...)
 	return nil, nil
 }
@@ -75,10 +66,10 @@ func (h *LoggingHook) After(ctx context.Context, hookContext of.HookContext,
 ) error {
 	args := h.buildArgs(hookContext)
 	args = append(args,
-		slog.String(REASON_KEY, string(flagEvaluationDetails.Reason)),
-		slog.String(VARIANT_KEY, flagEvaluationDetails.Variant),
-		slog.Any(VALUE_KEY, flagEvaluationDetails.Value),
-		slog.String(STAGE_KEY, "after"),
+		slog.String(reasonKey, string(flagEvaluationDetails.Reason)),
+		slog.String(variantKey, flagEvaluationDetails.Variant),
+		slog.Any(valueKey, flagEvaluationDetails.Value),
+		slog.String(stageKey, "after"),
 	)
 	h.logger.LogAttrs(ctx, slog.LevelDebug, "After stage", args...)
 	return nil
@@ -87,8 +78,8 @@ func (h *LoggingHook) After(ctx context.Context, hookContext of.HookContext,
 func (h *LoggingHook) Error(ctx context.Context, hookContext of.HookContext, err error, hookHints of.HookHints) {
 	args := h.buildArgs(hookContext)
 	args = append(args,
-		slog.Any(ERROR_MESSAGE_KEY, err),
-		slog.String(STAGE_KEY, "error"),
+		slog.Any(errorMessageKey, err),
+		slog.String(stageKey, "error"),
 	)
 	h.logger.LogAttrs(ctx, slog.LevelError, "Error stage", args...)
 }

openfeature/hooks/logging_hook_test.go

@@ -13,21 +13,15 @@ import (
 )
 
 func TestCreateLoggingHookWithDefaultLoggerAndContextInclusion(t *testing.T) {
-	hook, err := NewLoggingHook(true)
-	if err != nil {
-		t.Fatalf("expected no error, got %v", err)
-	}
+	hook := NewLoggingHook(true, slog.Default())
 	if hook == nil {
 		t.Fatal("expected a valid LoggingHook, got nil")
 	}
 }
 
 func TestLoggingHookInitializesCorrectly(t *testing.T) {
 	logger := slog.New(slog.NewJSONHandler(os.Stderr, nil))
-	hook, err := NewCustomLoggingHook(true, logger)
-	if err != nil {
-		t.Error("expected no error")
-	}
+	hook := NewLoggingHook(true, logger)
 
 	if hook.logger != logger {
 		t.Errorf("Expected logger to be %v, got %v", logger, hook.logger)
@@ -39,10 +33,7 @@ func TestLoggingHookInitializesCorrectly(t *testing.T) {
 }
 
 func TestLoggingHookHandlesNilLoggerGracefully(t *testing.T) {
-	hook, err := NewCustomLoggingHook(false, nil)
-	if err != nil {
-		t.Error("expected no error")
-	}
+	hook := NewLoggingHook(false, nil)
 
 	if hook.logger != nil {
 		t.Errorf("Expected logger to be nil, got %v", hook.logger)
@@ -58,24 +49,18 @@ func TestLoggingHookLogsMessagesAsExpected(t *testing.T) {
 	handler := slog.NewJSONHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})
 	logger := slog.New(handler)
 
-	hook, err := NewCustomLoggingHook(false, logger)
-	if err != nil {
-		t.Error("expected no error")
-	}
+	hook := NewLoggingHook(false, logger)
 
 	// Check if resultMap contains all key-value pairs from expected
 	testLoggingHookLogsMessagesAsExpected(*hook, logger, t, buf)
 }
 
 func TestLoggingHookLogsMessagesAsExpectedIncludeEvaluationContext(t *testing.T) {
-	var buf = new(bytes.Buffer)
+	buf := new(bytes.Buffer)
 	handler := slog.NewJSONHandler(buf, &slog.HandlerOptions{Level: slog.LevelDebug})
 	logger := slog.New(handler)
 
-	hook, err := NewCustomLoggingHook(true, logger)
-	if err != nil {
-		t.Error("expected no error")
-	}
+	hook := NewLoggingHook(true, logger)
 
 	// Check if resultMap contains all key-value pairs from expected
 	testLoggingHookLogsMessagesAsExpected(*hook, logger, t, buf)
@@ -184,7 +169,7 @@ func testLoggingHookLogsMessagesAsExpected(hook LoggingHook, logger *slog.Logger
 }
 
 func prepareOutput(buf *bytes.Buffer, t *testing.T) map[string]map[string]any {
-	var ms = make(map[string]map[string]any)
+	ms := make(map[string]map[string]any)
 	for _, line := range bytes.Split(buf.Bytes(), []byte{'\n'}) {
 		if len(line) == 0 {
 			continue
@@ -216,11 +201,11 @@ func compare(expected map[string]map[string]any, ms map[string]map[string]any, t
 			}
 
 			if hook.includeEvaluationContext {
-				evaluationContext, exists := resultInnerMap[EVALUATION_CONTEXT_KEY]
+				evaluationContext, exists := resultInnerMap[evaluationContextKey]
 				if !exists {
-					t.Errorf("Inner key %s not found in resultMap[%s]", EVALUATION_CONTEXT_KEY, key)
+					t.Errorf("Inner key %s not found in resultMap[%s]", evaluationContextKey, key)
 				}
-				attributes, attributesExists := evaluationContext.(map[string]any)["Attributes"]
+				attributes, attributesExists := evaluationContext.(map[string]any)[attributesKey]
 				if !attributesExists {
 					t.Errorf("attributes do not exist")
 				}
@@ -231,8 +216,8 @@ func compare(expected map[string]map[string]any, ms map[string]map[string]any, t
 				if color != "green" {
 					t.Errorf("expected green color in evaluationContext")
 				}
-				if evaluationContext.(map[string]any)["TargetingKey"] != "target1" {
-					t.Errorf("expected TargetingKey in evaluationContext")
+				if evaluationContext.(map[string]any)[targetingKeyKey] != "target1" {
+					t.Errorf("expected targeting_key in evaluationContext")
 				}
 			}
 		}