refactoring: fix the given review comments

router/core/access_log_field_handler_test.go

@@ -23,12 +23,11 @@ func TestAccessLogsFieldHandler(t *testing.T) {
 		rcc := buildRequestContext(requestContextOptions{r: req})
 		req = req.WithContext(withRequestContext(req.Context(), rcc))
 
-		response := AccessLogsFieldHandler(
+		response := RouterAccessLogsFieldHandler(
 			logger,
 			make([]config.CustomAttribute, 0),
 			make([]requestlogger.ExpressionAttribute, 0),
 			nil,
-			nil,
 			req,
 			nil,
 		)
@@ -56,12 +55,11 @@ func TestAccessLogsFieldHandler(t *testing.T) {
 			},
 		}
 
-		response := AccessLogsFieldHandler(
+		response := RouterAccessLogsFieldHandler(
 			logger,
 			make([]config.CustomAttribute, 0),
 			exprAttributes,
 			nil,
-			nil,
 			req,
 			nil,
 		)
@@ -102,12 +100,11 @@ func TestAccessLogsFieldHandler(t *testing.T) {
 			},
 		}
 
-		response := AccessLogsFieldHandler(
+		response := RouterAccessLogsFieldHandler(
 			logger,
 			make([]config.CustomAttribute, 0),
 			exprAttributes,
 			nil,
-			nil,
 			req,
 			nil,
 		)

router/core/context.go

@@ -255,29 +255,24 @@ type requestContext struct {
 
 func (c *requestContext) SetError(err error) {
 	c.error = err
-	if err != nil {
-		c.expressionContext.Request.Error = &ExprWrapError{err}
-	} else {
-		c.expressionContext.Request.Error = nil
-	}
+	c.expressionContext.Request.Error = err
 }
 
-func (c *requestContext) ResolveAnyExpressionWithErrorOverride(expression *vm.Program, err error) (any, error) {
-	currContextRequest := c.expressionContext.Request
-	if err != nil {
-		// We don't really want to do a deep clone since we only know that error only can get modified
-		copyContext := expr.Context{
+func (c *requestContext) ResolveAnyExpressionWithWrappedError(expression *vm.Program) (any, error) {
+	// If an error exists already, wrap it and resolve the expression with the copied context
+	currExprContext := c.expressionContext.Request
+	if currExprContext.Error != nil {
+		copyExprContext := expr.Context{
 			Request: expr.Request{
-				Auth:   currContextRequest.Auth,
-				URL:    currContextRequest.URL,
-				Header: currContextRequest.Header,
-				Error:  &ExprWrapError{err},
+				Auth:   currExprContext.Auth,
+				URL:    currExprContext.URL,
+				Header: currExprContext.Header,
+				Error:  &ExprWrapError{currExprContext.Error},
 			},
 		}
-		return expr.ResolveAnyExpression(expression, copyContext)
+		return expr.ResolveAnyExpression(expression, copyExprContext)
 	}
-
-	return c.ResolveAnyExpression(expression)
+	return expr.ResolveAnyExpression(expression, c.expressionContext)
 }
 
 func (c *requestContext) ResolveAnyExpression(expression *vm.Program) (any, error) {

router/core/graph_server.go

@@ -823,7 +823,7 @@ func (s *graphServer) buildGraphMux(ctx context.Context,
 			requestlogger.WithFields(baseLogFields...),
 			requestlogger.WithAttributes(s.accessLogsConfig.Attributes),
 			requestlogger.WithExprAttributes(exprAttributes),
-			requestlogger.WithFieldsHandler(AccessLogsFieldHandler),
+			requestlogger.WithFieldsHandler(RouterAccessLogsFieldHandler),
 		}
 
 		var ipAnonConfig *requestlogger.IPAnonymizationConfig
@@ -853,7 +853,7 @@ func (s *graphServer) buildGraphMux(ctx context.Context,
 				s.accessLogsConfig.Logger,
 				requestlogger.SubgraphOptions{
 					IPAnonymizationConfig: ipAnonConfig,
-					FieldsHandler:         AccessLogsFieldHandler,
+					FieldsHandler:         SubgraphAccessLogsFieldHandler,
 					Fields:                baseLogFields,
 					Attributes:            s.accessLogsConfig.SubgraphAttributes,
 					ExprAttributes:        exprAttributes,

router/core/header_rule_engine.go

@@ -358,7 +358,7 @@ func (h *HeaderPropagation) applyResponseRuleKeyValue(res *http.Response, propag
 func (h *HeaderPropagation) applyRequestRule(ctx RequestContext, request *http.Request, rule *config.RequestHeaderRule) {
 	if rule.Operation == config.HeaderRuleOperationSet {
 		if rule.ValueFrom != nil && rule.ValueFrom.ContextField != "" {
-			val := getCustomDynamicAttributeValue(rule.ValueFrom, getRequestContext(request.Context()), nil, nil)
+			val := getCustomDynamicAttributeValue(rule.ValueFrom, getRequestContext(request.Context()), nil)
 			value := fmt.Sprintf("%v", val)
 			if value != "" {
 				request.Header.Set(rule.Name, value)

router/core/request_context_fields.go

@@ -86,61 +86,94 @@ func NewDurationLogField(val time.Duration, attribute config.CustomAttribute) za
 	return zap.Skip()
 }
 
-func AccessLogsFieldHandler(
+func RouterAccessLogsFieldHandler(
 	logger *zap.Logger,
 	attributes []config.CustomAttribute,
 	exprAttributes []requestlogger.ExpressionAttribute,
-	passedErr error,
-	panicErr any,
+	passedErr any,
 	request *http.Request,
 	responseHeader *http.Header,
 ) []zapcore.Field {
 	resFields := make([]zapcore.Field, 0, len(attributes))
 
+	reqContext, resFields := processRequestIdField(request, resFields)
+	resFields = processCustomAttributes(attributes, responseHeader, resFields, request, reqContext, passedErr)
+	resFields = processExpressionAttributes(logger, exprAttributes, reqContext, resFields)
+
+	return resFields
+}
+
+func SubgraphAccessLogsFieldHandler(
+	_ *zap.Logger,
+	attributes []config.CustomAttribute,
+	_ []requestlogger.ExpressionAttribute,
+	passedErr any,
+	request *http.Request,
+	responseHeader *http.Header,
+) []zapcore.Field {
+	resFields := make([]zapcore.Field, 0, len(attributes))
+
+	reqContext, resFields := processRequestIdField(request, resFields)
+	resFields = processCustomAttributes(attributes, responseHeader, resFields, request, reqContext, passedErr)
+
+	return resFields
+}
+
+func processRequestIdField(request *http.Request, resFields []zapcore.Field) (*requestContext, []zapcore.Field) {
 	var reqContext *requestContext
-	if request != nil {
-		reqContext = getRequestContext(request.Context())
-		resFields = append(resFields, logging.WithRequestID(middleware.GetReqID(request.Context())))
+	if request == nil {
+		return reqContext, resFields
 	}
 
-	for _, field := range attributes {
-		if field.ValueFrom != nil && field.ValueFrom.ResponseHeader != "" && responseHeader != nil {
-			resFields = append(resFields, NewStringLogField(responseHeader.Get(field.ValueFrom.ResponseHeader), field))
-		} else if field.ValueFrom != nil && field.ValueFrom.RequestHeader != "" && request != nil {
-			resFields = append(resFields, NewStringLogField(request.Header.Get(field.ValueFrom.RequestHeader), field))
-		} else if field.ValueFrom != nil && field.ValueFrom.ContextField != "" {
-			if v := GetLogFieldFromCustomAttribute(field, reqContext, passedErr, panicErr); v != zap.Skip() {
-				resFields = append(resFields, v)
-			}
-		} else if field.Default != "" {
-			resFields = append(resFields, NewStringLogField(field.Default, field))
-		}
-	}
+	reqContext = getRequestContext(request.Context())
+	resFields = append(resFields, logging.WithRequestID(middleware.GetReqID(request.Context())))
+	return reqContext, resFields
+}
 
-	// If the request context was processed as nil (e.g. :- request was nil above)
+func processExpressionAttributes(logger *zap.Logger, exprAttributes []requestlogger.ExpressionAttribute, reqContext *requestContext, resFields []zapcore.Field) []zapcore.Field {
+	// If the request context was processed as nil (e.g. :- request was nil in the caller)
 	// do not proceed to process exprAttributes
 	if reqContext == nil {
 		return resFields
 	}
 
 	for _, exprField := range exprAttributes {
-		// TODO: We would ignore any panic recover errors being logged as of now
-		// if we want to do this we either need to type the expr.Error field as any OR
-		// add a separate field for this in the expr context (but I doubt how useful it will
-		// be for a client), just doing `request.error` is easy
-		result, err := reqContext.ResolveAnyExpressionWithErrorOverride(exprField.Expr, passedErr)
+		result, err := reqContext.ResolveAnyExpressionWithWrappedError(exprField.Expr)
 		if err != nil {
 			logger.Error("unable to process expression for access logs", zap.String("fieldKey", exprField.Key), zap.Error(err))
 			continue
 		}
 		resFields = append(resFields, NewExpressionLogField(result, exprField.Key, exprField.Default))
 	}
+	return resFields
+}
 
+func processCustomAttributes(
+	attributes []config.CustomAttribute,
+	responseHeader *http.Header,
+	resFields []zapcore.Field,
+	request *http.Request,
+	reqContext *requestContext,
+	passedErr any,
+) []zapcore.Field {
+	for _, field := range attributes {
+		if field.ValueFrom != nil && field.ValueFrom.ResponseHeader != "" && responseHeader != nil {
+			resFields = append(resFields, NewStringLogField(responseHeader.Get(field.ValueFrom.ResponseHeader), field))
+		} else if field.ValueFrom != nil && field.ValueFrom.RequestHeader != "" && request != nil {
+			resFields = append(resFields, NewStringLogField(request.Header.Get(field.ValueFrom.RequestHeader), field))
+		} else if field.ValueFrom != nil && field.ValueFrom.ContextField != "" {
+			if v := GetLogFieldFromCustomAttribute(field, reqContext, passedErr); v != zap.Skip() {
+				resFields = append(resFields, v)
+			}
+		} else if field.Default != "" {
+			resFields = append(resFields, NewStringLogField(field.Default, field))
+		}
+	}
 	return resFields
 }
 
-func GetLogFieldFromCustomAttribute(field config.CustomAttribute, req *requestContext, err error, panicErr any) zap.Field {
-	val := getCustomDynamicAttributeValue(field.ValueFrom, req, err, panicErr)
+func GetLogFieldFromCustomAttribute(field config.CustomAttribute, req *requestContext, err any) zap.Field {
+	val := getCustomDynamicAttributeValue(field.ValueFrom, req, err)
 	switch v := val.(type) {
 	case string:
 		return NewStringLogField(v, field)
@@ -158,8 +191,7 @@ func GetLogFieldFromCustomAttribute(field config.CustomAttribute, req *requestCo
 func getCustomDynamicAttributeValue(
 	attribute *config.CustomDynamicAttribute,
 	reqContext *requestContext,
-	err error,
-	panicErr any,
+	err any,
 ) interface{} {
 	if attribute == nil || attribute.ContextField == "" {
 		return ""
@@ -177,7 +209,7 @@ func getCustomDynamicAttributeValue(
 
 	switch attribute.ContextField {
 	case ContextFieldRequestError:
-		return err != nil || panicErr != nil || reqContext.error != nil
+		return err != nil || reqContext.error != nil
 	case ContextFieldOperationName:
 		return reqContext.operation.Name()
 	case ContextFieldOperationType:
@@ -203,9 +235,6 @@ func getCustomDynamicAttributeValue(
 		if err != nil {
 			return fmt.Sprintf("%v", err)
 		}
-		if panicErr != nil {
-			return fmt.Sprintf("%v", panicErr)
-		}
 		if reqContext.error != nil {
 			return reqContext.error.Error()
 		}

router/internal/requestlogger/requestlogger.go

@@ -17,7 +17,14 @@ import (
 	"go.uber.org/zap/zapcore"
 )
 
-type ContextFunc func(logger *zap.Logger, fields []config.CustomAttribute, exprFields []ExpressionAttribute, err error, panic any, r *http.Request, rh *http.Header) []zapcore.Field
+type ContextFunc func(
+	logger *zap.Logger,
+	fields []config.CustomAttribute,
+	exprFields []ExpressionAttribute,
+	err any,
+	r *http.Request,
+	rh *http.Header,
+) []zapcore.Field
 
 // Option provides a functional approach to define
 // configuration for a handler; such as setting the logging
@@ -139,7 +146,7 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			// This is only called on panic so it is safe to call it here again
 			// to gather all the fields that are needed for logging
 			if h.accessLogger.fieldsHandler != nil {
-				fields = append(fields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, nil, err, r, nil)...)
+				fields = append(fields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, err, r, nil)...)
 			}
 
 			if brokenPipe {
@@ -168,7 +175,7 @@ func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if h.accessLogger.fieldsHandler != nil {
-		resFields = append(resFields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, nil, nil, r, nil)...)
+		resFields = append(resFields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, nil, r, nil)...)
 	}
 
 	h.logger.Info(path, append(fields, resFields...)...)

router/internal/requestlogger/subgraphlogger.go

@@ -56,7 +56,7 @@ func (h *SubgraphAccessLogger) RequestFields(respInfo *resolve.ResponseInfo, sub
 		fields = append(fields, zap.String("url", respInfo.Request.URL.String()))
 	}
 	if h.accessLogger.fieldsHandler != nil {
-		fields = append(fields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, respInfo.Err, nil, respInfo.Request, &respInfo.ResponseHeaders)...)
+		fields = append(fields, h.accessLogger.fieldsHandler(h.logger, h.accessLogger.attributes, h.accessLogger.exprAttributes, respInfo.Err, respInfo.Request, &respInfo.ResponseHeaders)...)
 	}
 
 	return fields

router/internal/requestlogger/subgraphlogger_test.go

@@ -148,7 +148,7 @@ func TestSubgraphAccessLogger(t *testing.T) {
 		l := logging.NewZapLoggerWithCore(zCore, true)
 
 		subgraphLogger := requestlogger.NewSubgraphAccessLogger(l, requestlogger.SubgraphOptions{
-			FieldsHandler: core.AccessLogsFieldHandler,
+			FieldsHandler: core.SubgraphAccessLogsFieldHandler,
 			Attributes: []config.CustomAttribute{
 				{
 					Key: "test",
@@ -198,7 +198,7 @@ func TestSubgraphAccessLogger(t *testing.T) {
 		l := logging.NewZapLoggerWithCore(zCore, true)
 
 		subgraphLogger := requestlogger.NewSubgraphAccessLogger(l, requestlogger.SubgraphOptions{
-			FieldsHandler: core.AccessLogsFieldHandler,
+			FieldsHandler: core.SubgraphAccessLogsFieldHandler,
 			Attributes: []config.CustomAttribute{
 				{
 					Key: "test",

router/pkg/config/config.schema.json

@@ -553,7 +553,25 @@
           "description": "The configuration for the router access logs",
           "properties": {
             "fields": {
-              "$ref": "#/definitions/context_fields"
+              "allOf": [
+                {
+                  "$ref": "#/definitions/context_fields"
+                },
+                {
+                  "items": {
+                    "properties": {
+                      "value_from": {
+                        "properties": {
+                          "expression": {
+                            "type": "string",
+                            "description": "The expression used to evaluate to extract a value for logging. The expression is specified as a string. Please see https://expr-lang.org/ for more information on constructing expressions."
+                          }
+                        }
+                      }
+                    }
+                  }
+                }
+              ]
             }
           }
         },
@@ -821,10 +839,6 @@
                         "type": "string",
                         "description": "The name of the request header from which to extract the value. The value is only extracted when a request context is available otherwise the default value is used."
                       },
-                      "expression": {
-                        "type": "string",
-                        "description": "The expression used to evaluate to extract a value for logging. The expression is specified as a string. Please see https://expr-lang.org/ for more information on constructing expressions."
-                      },
                       "context_field": {
                         "type": "string",
                         "description": "The field name of the context from which to extract the value. The value is only extracted when a context is available otherwise the default value is used.",
@@ -2558,10 +2572,6 @@
                 "type": "string",
                 "description": "The name of the response header from which to extract the value. The value is only extracted for subgraph access logs"
               },
-              "expression": {
-                "type": "string",
-                "description": "The expression used to evaluate to extract a value for logging. The expression is specified as a string. Please see https://expr-lang.org/ for more information on constructing expressions."
-              },
               "context_field": {
                 "type": "string",
                 "description": "The field name of the context from which to extract the value. The value is only extracted when a context is available otherwise the default value is used.",