Merge pull request #4935 from butonic/log-uploads

pass proper logger down to datatx handler and tus
cs3org · Nov 19, 2024 · 6f05da6 · 6f05da6
2 parents 0cdf21e + 4afdddc
commit 6f05da6
Show file tree

Hide file tree

Showing 6 changed files with 60 additions and 23 deletions.
diff --git a/changelog/unreleased/log-uploads.md b/changelog/unreleased/log-uploads.md
@@ -0,0 +1,6 @@
+Bugfix: enable datatx log
+
+We now pass a properly initialized logger to the datatx implementations, allowing the tus handler to log with the same level as the rest of reva.
+
+https://github.com/cs3org/reva/pull/4935
+
diff --git a/internal/http/services/dataprovider/dataprovider.go b/internal/http/services/dataprovider/dataprovider.go
@@ -22,6 +22,9 @@ import (
 	"fmt"
 	"net/http"
 
+	"github.com/mitchellh/mapstructure"
+	"github.com/rs/zerolog"
+
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	"github.com/cs3org/reva/v2/pkg/events"
 	"github.com/cs3org/reva/v2/pkg/events/stream"
@@ -30,8 +33,6 @@ import (
 	"github.com/cs3org/reva/v2/pkg/rhttp/router"
 	"github.com/cs3org/reva/v2/pkg/storage"
 	"github.com/cs3org/reva/v2/pkg/storage/fs/registry"
-	"github.com/mitchellh/mapstructure"
-	"github.com/rs/zerolog"
 )
 
 func init() {
@@ -103,7 +104,7 @@ func New(m map[string]interface{}, log *zerolog.Logger) (global.Service, error)
 		return nil, err
 	}
 
-	dataTXs, err := getDataTXs(conf, fs, evstream)
+	dataTXs, err := getDataTXs(conf, fs, evstream, log)
 	if err != nil {
 		return nil, err
 	}
@@ -125,7 +126,7 @@ func getFS(c *config, stream events.Stream, log *zerolog.Logger) (storage.FS, er
 	return nil, fmt.Errorf("driver not found: %s", c.Driver)
 }
 
-func getDataTXs(c *config, fs storage.FS, publisher events.Publisher) (map[string]http.Handler, error) {
+func getDataTXs(c *config, fs storage.FS, publisher events.Publisher, log *zerolog.Logger) (map[string]http.Handler, error) {
 	if c.DataTXs == nil {
 		c.DataTXs = make(map[string]map[string]interface{})
 	}
@@ -144,7 +145,7 @@ func getDataTXs(c *config, fs storage.FS, publisher events.Publisher) (map[strin
 	txs := make(map[string]http.Handler)
 	for t := range c.DataTXs {
 		if f, ok := datatxregistry.NewFuncs[t]; ok {
-			if tx, err := f(c.DataTXs[t], publisher); err == nil {
+			if tx, err := f(c.DataTXs[t], publisher, log); err == nil {
 				if handler, err := tx.Handler(fs); err == nil {
 					txs[t] = handler
 				}

diff --git a/pkg/rhttp/datatx/manager/registry/registry.go b/pkg/rhttp/datatx/manager/registry/registry.go
@@ -19,13 +19,15 @@
 package registry
 
 import (
+	"github.com/rs/zerolog"
+
 	"github.com/cs3org/reva/v2/pkg/events"
 	"github.com/cs3org/reva/v2/pkg/rhttp/datatx"
 )
 
 // NewFunc is the function that data transfer implementations
 // should register at init time.
-type NewFunc func(map[string]interface{}, events.Publisher) (datatx.DataTX, error)
+type NewFunc func(map[string]interface{}, events.Publisher, *zerolog.Logger) (datatx.DataTX, error)
 
 // NewFuncs is a map containing all the registered data transfers.
 var NewFuncs = map[string]NewFunc{}

diff --git a/pkg/rhttp/datatx/manager/simple/simple.go b/pkg/rhttp/datatx/manager/simple/simple.go
@@ -22,11 +22,12 @@ import (
 	"net/http"
 	"time"
 
+	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
+	"github.com/rs/zerolog"
 
-	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
-	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	"github.com/cs3org/reva/v2/pkg/errtypes"
@@ -48,6 +49,7 @@ func init() {
 type manager struct {
 	conf      *cache.Config
 	publisher events.Publisher
+	log       *zerolog.Logger
 }
 
 func parseConfig(m map[string]interface{}) (*cache.Config, error) {
@@ -60,22 +62,26 @@ func parseConfig(m map[string]interface{}) (*cache.Config, error) {
 }
 
 // New returns a datatx manager implementation that relies on HTTP PUT/GET.
-func New(m map[string]interface{}, publisher events.Publisher) (datatx.DataTX, error) {
+func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logger) (datatx.DataTX, error) {
 	c, err := parseConfig(m)
 	if err != nil {
 		return nil, err
 	}
 
+	l := log.With().Str("datatx", "simple").Logger()
+
 	return &manager{
 		conf:      c,
 		publisher: publisher,
+		log:       &l,
 	}, nil
 }
 
 func (m *manager) Handler(fs storage.FS) (http.Handler, error) {
 	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		sublog := m.log.With().Str("path", r.URL.Path).Logger()
+		r = r.WithContext(appctx.WithLogger(r.Context(), &sublog))
 		ctx := r.Context()
-		sublog := appctx.GetLogger(ctx).With().Str("datatx", "simple").Logger()
 
 		switch r.Method {
 		case "GET", "HEAD":

diff --git a/pkg/rhttp/datatx/manager/spaces/spaces.go b/pkg/rhttp/datatx/manager/spaces/spaces.go
@@ -26,6 +26,10 @@ import (
 
 	userpb "github.com/cs3org/go-cs3apis/cs3/identity/user/v1beta1"
 	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/mitchellh/mapstructure"
+	"github.com/pkg/errors"
+	"github.com/rs/zerolog"
+
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	"github.com/cs3org/reva/v2/pkg/errtypes"
@@ -39,8 +43,6 @@ import (
 	"github.com/cs3org/reva/v2/pkg/storage/cache"
 	"github.com/cs3org/reva/v2/pkg/storagespace"
 	"github.com/cs3org/reva/v2/pkg/utils"
-	"github.com/mitchellh/mapstructure"
-	"github.com/pkg/errors"
 )
 
 func init() {
@@ -50,6 +52,7 @@ func init() {
 type manager struct {
 	conf      *cache.Config
 	publisher events.Publisher
+	log       *zerolog.Logger
 }
 
 func parseConfig(m map[string]interface{}) (*cache.Config, error) {
@@ -62,25 +65,29 @@ func parseConfig(m map[string]interface{}) (*cache.Config, error) {
 }
 
 // New returns a datatx manager implementation that relies on HTTP PUT/GET.
-func New(m map[string]interface{}, publisher events.Publisher) (datatx.DataTX, error) {
+func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logger) (datatx.DataTX, error) {
 	c, err := parseConfig(m)
 	if err != nil {
 		return nil, err
 	}
 
+	l := log.With().Str("datatx", "spaces").Logger()
+
 	return &manager{
 		conf:      c,
 		publisher: publisher,
+		log:       &l,
 	}, nil
 }
 
 func (m *manager) Handler(fs storage.FS) (http.Handler, error) {
 	h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		ctx := r.Context()
 		var spaceID string
 		spaceID, r.URL.Path = router.ShiftPath(r.URL.Path)
 
-		sublog := appctx.GetLogger(ctx).With().Str("datatx", "spaces").Str("spaceid", spaceID).Logger()
+		sublog := m.log.With().Str("spaceid", spaceID).Str("path", r.URL.Path).Logger()
+		r = r.WithContext(appctx.WithLogger(r.Context(), &sublog))
+		ctx := r.Context()
 
 		switch r.Method {
 		case "GET", "HEAD":

diff --git a/pkg/rhttp/datatx/manager/tus/tus.go b/pkg/rhttp/datatx/manager/tus/tus.go
@@ -24,11 +24,13 @@ import (
 	"path"
 	"regexp"
 
+	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
+	"github.com/mitchellh/mapstructure"
 	"github.com/pkg/errors"
 	"github.com/rs/zerolog"
 	tusd "github.com/tus/tusd/v2/pkg/handler"
+	"golang.org/x/exp/slog"
 
-	provider "github.com/cs3org/go-cs3apis/cs3/storage/provider/v1beta1"
 	"github.com/cs3org/reva/v2/internal/http/services/owncloud/ocdav/net"
 	"github.com/cs3org/reva/v2/pkg/appctx"
 	"github.com/cs3org/reva/v2/pkg/errtypes"
@@ -38,8 +40,6 @@ import (
 	"github.com/cs3org/reva/v2/pkg/rhttp/datatx/metrics"
 	"github.com/cs3org/reva/v2/pkg/storage"
 	"github.com/cs3org/reva/v2/pkg/storagespace"
-	"github.com/mitchellh/mapstructure"
-	"golang.org/x/exp/slog"
 )
 
 func init() {
@@ -59,6 +59,7 @@ type TusConfig struct {
 type manager struct {
 	conf      *TusConfig
 	publisher events.Publisher
+	log       *zerolog.Logger
 }
 
 func parseConfig(m map[string]interface{}) (*TusConfig, error) {
@@ -71,14 +72,18 @@ func parseConfig(m map[string]interface{}) (*TusConfig, error) {
 }
 
 // New returns a datatx manager implementation that relies on HTTP PUT/GET.
-func New(m map[string]interface{}, publisher events.Publisher) (datatx.DataTX, error) {
+func New(m map[string]interface{}, publisher events.Publisher, log *zerolog.Logger) (datatx.DataTX, error) {
 	c, err := parseConfig(m)
 	if err != nil {
 		return nil, err
 	}
+
+	l := log.With().Str("datatx", "tus").Logger()
+
 	return &manager{
 		conf:      c,
 		publisher: publisher,
+		log:       &l,
 	}, nil
 }
 
@@ -100,7 +105,7 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) {
 	config := tusd.Config{
 		StoreComposer:         composer,
 		NotifyCompleteUploads: true,
-		Logger:                slog.New(tusdLogger{log: appctx.GetLogger(context.Background())}), // Note: this is a noop logger
+		Logger:                slog.New(tusdLogger{log: m.log}),
 	}
 
 	if m.conf.CorsEnabled {
@@ -154,6 +159,8 @@ func (m *manager) Handler(fs storage.FS) (http.Handler, error) {
 	}
 
 	h := handler.Middleware(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		sublog := m.log.With().Str("uploadid", r.URL.Path).Logger()
+		r = r.WithContext(appctx.WithLogger(r.Context(), &sublog))
 		method := r.Method
 		// https://github.com/tus/tus-resumable-upload-protocol/blob/master/protocol.md#x-http-method-override
 		if r.Header.Get("X-HTTP-Method-Override") != "" {
@@ -251,8 +258,16 @@ func (l tusdLogger) Handle(_ context.Context, r slog.Record) error {
 // Enabled returns true
 func (l tusdLogger) Enabled(_ context.Context, _ slog.Level) bool { return true }
 
-// WithAttrs is not implemented
-func (l tusdLogger) WithAttrs(_ []slog.Attr) slog.Handler { return l }
+// WithAttrs creates a new logger with the given attributes
+func (l tusdLogger) WithAttrs(attr []slog.Attr) slog.Handler {
+	fields := make(map[string]interface{}, len(attr))
+	for _, a := range attr {
+		fields[a.Key] = a.Value
+	}
+	c := l.log.With().Fields(fields).Logger()
+	sLog := tusdLogger{log: &c}
+	return sLog
+}
 
 // WithGroup is not implemented
-func (l tusdLogger) WithGroup(_ string) slog.Handler { return l }
+func (l tusdLogger) WithGroup(name string) slog.Handler { return l }