Merge pull request #549 from router-for-me/log

Improve Request Logging Efficiency and Standardize Error Responses

internal/api/middleware/response_writer.go

@@ -71,22 +71,64 @@ func (w *ResponseWriterWrapper) Write(data []byte) (int, error) {
 	n, err := w.ResponseWriter.Write(data)
 
 	// THEN: Handle logging based on response type
-	if w.isStreaming {
+	if w.isStreaming && w.chunkChannel != nil {
 		// For streaming responses: Send to async logging channel (non-blocking)
-		if w.chunkChannel != nil {
-			select {
-			case w.chunkChannel <- append([]byte(nil), data...): // Non-blocking send with copy
-			default: // Channel full, skip logging to avoid blocking
-			}
+		select {
+		case w.chunkChannel <- append([]byte(nil), data...): // Non-blocking send with copy
+		default: // Channel full, skip logging to avoid blocking
 		}
-	} else {
-		// For non-streaming responses: Buffer complete response
+		return n, err
+	}
+
+	if w.shouldBufferResponseBody() {
 		w.body.Write(data)
 	}
 
 	return n, err
 }
 
+func (w *ResponseWriterWrapper) shouldBufferResponseBody() bool {
+	if w.logger != nil && w.logger.IsEnabled() {
+		return true
+	}
+	if !w.logOnErrorOnly {
+		return false
+	}
+	status := w.statusCode
+	if status == 0 {
+		if statusWriter, ok := w.ResponseWriter.(interface{ Status() int }); ok && statusWriter != nil {
+			status = statusWriter.Status()
+		} else {
+			status = http.StatusOK
+		}
+	}
+	return status >= http.StatusBadRequest
+}
+
+// WriteString wraps the underlying ResponseWriter's WriteString method to capture response data.
+// Some handlers (and fmt/io helpers) write via io.StringWriter; without this override, those writes
+// bypass Write() and would be missing from request logs.
+func (w *ResponseWriterWrapper) WriteString(data string) (int, error) {
+	w.ensureHeadersCaptured()
+
+	// CRITICAL: Write to client first (zero latency)
+	n, err := w.ResponseWriter.WriteString(data)
+
+	// THEN: Capture for logging
+	if w.isStreaming && w.chunkChannel != nil {
+		select {
+		case w.chunkChannel <- []byte(data):
+		default:
+		}
+		return n, err
+	}
+
+	if w.shouldBufferResponseBody() {
+		w.body.WriteString(data)
+	}
+	return n, err
+}
+
 // WriteHeader wraps the underlying ResponseWriter's WriteHeader method.
 // It captures the status code, detects if the response is streaming based on the Content-Type header,
 // and initializes the appropriate logging mechanism (standard or streaming).
@@ -160,12 +202,16 @@ func (w *ResponseWriterWrapper) detectStreaming(contentType string) bool {
 		return true
 	}
 
-	// Check request body for streaming indicators
-	if w.requestInfo.Body != nil {
+	// If a concrete Content-Type is already set (e.g., application/json for error responses),
+	// treat it as non-streaming instead of inferring from the request payload.
+	if strings.TrimSpace(contentType) != "" {
+		return false
+	}
+
+	// Only fall back to request payload hints when Content-Type is not set yet.
+	if w.requestInfo != nil && len(w.requestInfo.Body) > 0 {
 		bodyStr := string(w.requestInfo.Body)
-		if strings.Contains(bodyStr, `"stream": true`) || strings.Contains(bodyStr, `"stream":true`) {
-			return true
-		}
+		return strings.Contains(bodyStr, `"stream": true`) || strings.Contains(bodyStr, `"stream":true`)
 	}
 
 	return false
@@ -221,7 +267,7 @@ func (w *ResponseWriterWrapper) Finalize(c *gin.Context) error {
 		return nil
 	}
 
-	if w.isStreaming {
+	if w.isStreaming && w.streamWriter != nil {
 		if w.chunkChannel != nil {
 			close(w.chunkChannel)
 			w.chunkChannel = nil
@@ -233,24 +279,19 @@ func (w *ResponseWriterWrapper) Finalize(c *gin.Context) error {
 		}
 
 		// Write API Request and Response to the streaming log before closing
-		if w.streamWriter != nil {
-			apiRequest := w.extractAPIRequest(c)
-			if len(apiRequest) > 0 {
-				_ = w.streamWriter.WriteAPIRequest(apiRequest)
-			}
-			apiResponse := w.extractAPIResponse(c)
-			if len(apiResponse) > 0 {
-				_ = w.streamWriter.WriteAPIResponse(apiResponse)
-			}
-			if err := w.streamWriter.Close(); err != nil {
-				w.streamWriter = nil
-				return err
-			}
+		apiRequest := w.extractAPIRequest(c)
+		if len(apiRequest) > 0 {
+			_ = w.streamWriter.WriteAPIRequest(apiRequest)
+		}
+		apiResponse := w.extractAPIResponse(c)
+		if len(apiResponse) > 0 {
+			_ = w.streamWriter.WriteAPIResponse(apiResponse)
+		}
+		if err := w.streamWriter.Close(); err != nil {
 			w.streamWriter = nil
+			return err
 		}
-		if forceLog {
-			return w.logRequest(finalStatusCode, w.cloneHeaders(), w.body.Bytes(), w.extractAPIRequest(c), w.extractAPIResponse(c), slicesAPIResponseError, forceLog)
-		}
+		w.streamWriter = nil
 		return nil
 	}
 
@@ -335,26 +376,3 @@ func (w *ResponseWriterWrapper) logRequest(statusCode int, headers map[string][]
 		apiResponseErrors,
 	)
 }
-
-// Status returns the HTTP response status code captured by the wrapper.
-// It defaults to 200 if WriteHeader has not been called.
-func (w *ResponseWriterWrapper) Status() int {
-	if w.statusCode == 0 {
-		return 200 // Default status code
-	}
-	return w.statusCode
-}
-
-// Size returns the size of the response body in bytes for non-streaming responses.
-// For streaming responses, it returns -1, as the total size is unknown.
-func (w *ResponseWriterWrapper) Size() int {
-	if w.isStreaming {
-		return -1 // Unknown size for streaming responses
-	}
-	return w.body.Len()
-}
-
-// Written returns true if the response header has been written (i.e., a status code has been set).
-func (w *ResponseWriterWrapper) Written() bool {
-	return w.statusCode != 0
-}

sdk/api/handlers/handlers.go

@@ -5,6 +5,7 @@ package handlers
 
 import (
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"strings"
@@ -117,6 +118,16 @@ func (h *BaseAPIHandler) GetContextWithCancel(handler interfaces.APIHandler, c *
 	newCtx = context.WithValue(newCtx, "handler", handler)
 	return newCtx, func(params ...interface{}) {
 		if h.Cfg.RequestLog && len(params) == 1 {
+			if existing, exists := c.Get("API_RESPONSE"); exists {
+				if existingBytes, ok := existing.([]byte); ok && len(bytes.TrimSpace(existingBytes)) > 0 {
+					switch params[0].(type) {
+					case error, string:
+						cancel()
+						return
+					}
+				}
+			}
+
 			var payload []byte
 			switch data := params[0].(type) {
 			case []byte:
@@ -437,12 +448,53 @@ func (h *BaseAPIHandler) WriteErrorResponse(c *gin.Context, msg *interfaces.Erro
 			}
 		}
 	}
-	c.Status(status)
+
+	errText := http.StatusText(status)
 	if msg != nil && msg.Error != nil {
-		_, _ = c.Writer.Write([]byte(msg.Error.Error()))
-	} else {
-		_, _ = c.Writer.Write([]byte(http.StatusText(status)))
+		if v := strings.TrimSpace(msg.Error.Error()); v != "" {
+			errText = v
+		}
 	}
+
+	// Prefer preserving upstream JSON error bodies when possible.
+	buildJSONBody := func() []byte {
+		trimmed := strings.TrimSpace(errText)
+		if trimmed != "" && json.Valid([]byte(trimmed)) {
+			return []byte(trimmed)
+		}
+		errType := "invalid_request_error"
+		switch status {
+		case http.StatusUnauthorized:
+			errType = "authentication_error"
+		case http.StatusForbidden:
+			errType = "permission_error"
+		case http.StatusTooManyRequests:
+			errType = "rate_limit_error"
+		default:
+			if status >= http.StatusInternalServerError {
+				errType = "server_error"
+			}
+		}
+		payload, err := json.Marshal(ErrorResponse{
+			Error: ErrorDetail{
+				Message: errText,
+				Type:    errType,
+			},
+		})
+		if err != nil {
+			return []byte(fmt.Sprintf(`{"error":{"message":%q,"type":"server_error"}}`, errText))
+		}
+		return payload
+	}
+
+	body := buildJSONBody()
+	c.Set("API_RESPONSE", bytes.Clone(body))
+
+	if !c.Writer.Written() {
+		c.Writer.Header().Set("Content-Type", "application/json")
+	}
+	c.Status(status)
+	_, _ = c.Writer.Write(body)
 }
 
 func (h *BaseAPIHandler) LoggingAPIResponseError(ctx context.Context, err *interfaces.ErrorMessage) {