From bec38d900639346fb567994cc99ebf283dd94cf6 Mon Sep 17 00:00:00 2001 From: Krishna Iyer Easwaran Date: Mon, 6 Nov 2023 13:04:27 +0100 Subject: [PATCH] all: Harmonize logger usage --- cmd/root.go | 16 ----------- pkg/commands/source.go | 1 - pkg/log/log.go | 43 +++++++++++++++++++++++++++++ pkg/source/firefly/client/client.go | 20 ++++++++------ pkg/source/firefly/config.go | 13 --------- pkg/source/firefly/source.go | 9 +++--- pkg/source/source.go | 12 ++++++++ pkg/source/tts/config/config.go | 25 ++--------------- pkg/source/tts/source.go | 25 ++++++++--------- pkg/source/tts/tts.go | 2 -- 10 files changed, 84 insertions(+), 82 deletions(-) create mode 100644 pkg/log/log.go diff --git a/cmd/root.go b/cmd/root.go index e46fd77..04294c9 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -26,7 +26,6 @@ import ( "go.thethings.network/lorawan-stack-migrate/pkg/export" "go.thethings.network/lorawan-stack-migrate/pkg/source" "go.thethings.network/lorawan-stack/v3/pkg/log" - "go.thethings.network/lorawan-stack/v3/pkg/rpcmiddleware/rpclog" ) var ( @@ -40,25 +39,10 @@ var ( SilenceUsage: true, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - logLevel := log.InfoLevel - if verbose, _ := cmd.Flags().GetBool("verbose"); verbose { - logLevel = log.DebugLevel - } - logHandler, err := log.NewZap("console") - if err != nil { - return err - } - logger = log.NewLogger( - logHandler, - log.WithLevel(logLevel), - ) - rpclog.ReplaceGrpcLogger(logger) ctx = log.NewContext(ctx, logger) exportCfg.DevIDPrefix, _ = cmd.Flags().GetString("dev-id-prefix") - exportCfg.EUIForID, _ = cmd.Flags().GetBool("set-eui-as-id") ctx = export.NewContext(ctx, exportCfg) - cmd.SetContext(ctx) return nil }, diff --git a/pkg/commands/source.go b/pkg/commands/source.go index 2283100..454f932 100644 --- a/pkg/commands/source.go +++ b/pkg/commands/source.go @@ -127,7 +127,6 @@ func ExecuteParentPersistentPreRun(cmd *cobra.Command, args []string) error { return nil } p := cmd.Parent() - if f := p.PersistentPreRunE; f != nil { if err := f(p, args); err != nil { return err diff --git a/pkg/log/log.go b/pkg/log/log.go new file mode 100644 index 0000000..11f09b2 --- /dev/null +++ b/pkg/log/log.go @@ -0,0 +1,43 @@ +// Copyright © 2023 The Things Network Foundation, The Things Industries B.V. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package log + +import ( + "context" + + "go.uber.org/zap" +) + +type loggerKeyType string + +var loggerKey loggerKeyType = "logger" + +// NewContext returns a new context with a *zap.SugaredLogger and panics if the logger is nil. +func NewContext(parentCtx context.Context, logger *zap.SugaredLogger) context.Context { + if logger == nil { + panic("Nil logger") + } + return context.WithValue(parentCtx, loggerKey, logger) +} + +// FromContext retrieves a *zap.SugaredLogger from a context and panics if there isn't one. +func FromContext(ctx context.Context) *zap.SugaredLogger { + val := ctx.Value(loggerKey) + logger, ok := val.(*zap.SugaredLogger) + if !ok { + panic("No logger in context") + } + return logger +} diff --git a/pkg/source/firefly/client/client.go b/pkg/source/firefly/client/client.go index fde62c6..3d7fb50 100644 --- a/pkg/source/firefly/client/client.go +++ b/pkg/source/firefly/client/client.go @@ -16,6 +16,7 @@ package client import ( "bytes" + "context" "crypto/tls" "crypto/x509" "encoding/json" @@ -26,8 +27,8 @@ import ( "os" "time" + "go.thethings.network/lorawan-stack-migrate/pkg/log" "go.thethings.network/lorawan-stack/v3/pkg/errors" - "go.uber.org/zap" ) const defaultTimeout = 10 * time.Second @@ -44,11 +45,11 @@ type Config struct { type Client struct { *Config *http.Client - logger *zap.SugaredLogger + ctx context.Context } // New creates a new Firefly client. -func (cfg *Config) NewClient(logger *zap.SugaredLogger) (*Client, error) { +func (cfg *Config) NewClient(ctx context.Context) (*Client, error) { httpTransport := &http.Transport{} if cfg.CACertPath != "" && !cfg.UseHTTP { pemBytes, err := os.ReadFile(cfg.CACertPath) @@ -72,7 +73,7 @@ func (cfg *Config) NewClient(logger *zap.SugaredLogger) (*Client, error) { Transport: httpTransport, Timeout: defaultTimeout, }, - logger: logger, + ctx: ctx, }, nil } @@ -100,7 +101,8 @@ func (c *Client) do(resource, method string, body []byte, params string) ([]byte Path: fmt.Sprintf("/api/v1/%s", resource), RawQuery: authAndParams, } - logger := c.logger.With("url", url.String()) + + logger := log.FromContext(c.ctx).With("url", url.String()) logger.Debug("Request resource") req, err := http.NewRequest(method, url.String(), bytes.NewReader(body)) req.Header.Set("Content-Type", "application/json") @@ -112,12 +114,12 @@ func (c *Client) do(resource, method string, body []byte, params string) ([]byte return nil, err } defer res.Body.Close() + body, err = io.ReadAll(res.Body) + if err != nil { + return nil, err + } switch { case res.StatusCode == http.StatusOK: - body, err := io.ReadAll(res.Body) - if err != nil { - return nil, err - } return body, nil case res.StatusCode == http.StatusNotFound: return nil, errResourceNotFound.WithAttributes("resource", resource) diff --git a/pkg/source/firefly/config.go b/pkg/source/firefly/config.go index 6c46775..cc6cb7f 100644 --- a/pkg/source/firefly/config.go +++ b/pkg/source/firefly/config.go @@ -18,7 +18,6 @@ import ( "os" "github.com/spf13/pflag" - "go.uber.org/zap" "go.thethings.network/lorawan-stack-migrate/pkg/source" "go.thethings.network/lorawan-stack-migrate/pkg/source/firefly/client" @@ -42,8 +41,6 @@ type Config struct { flags *pflag.FlagSet } -var logger *zap.SugaredLogger - // NewConfig returns a new Firefly configuration. func NewConfig() *Config { config := &Config{ @@ -101,16 +98,6 @@ where the devices are exported but they are still valid on the firefly server // Initialize the configuration. func (c *Config) Initialize(src source.Config) error { c.src = src - cfg := zap.NewProductionConfig() - if c.src.Verbose { - cfg.Level = zap.NewAtomicLevelAt(zap.DebugLevel) - } - zapLogger, err := cfg.Build() - if err != nil { - return err - } - logger = zapLogger.Sugar() - if c.appID == "" { return errNoAppID.New() } diff --git a/pkg/source/firefly/source.go b/pkg/source/firefly/source.go index db471e8..0a0e376 100644 --- a/pkg/source/firefly/source.go +++ b/pkg/source/firefly/source.go @@ -26,6 +26,7 @@ import ( "google.golang.org/protobuf/types/known/wrapperspb" "go.thethings.network/lorawan-stack-migrate/pkg/iterator" + "go.thethings.network/lorawan-stack-migrate/pkg/log" "go.thethings.network/lorawan-stack-migrate/pkg/source" "go.thethings.network/lorawan-stack-migrate/pkg/source/firefly/client" "go.thethings.network/lorawan-stack-migrate/pkg/util" @@ -41,7 +42,7 @@ func createNewSource(cfg *Config) source.CreateSource { if err := cfg.Initialize(src); err != nil { return nil, err } - client, err := cfg.NewClient(logger) + client, err := cfg.NewClient(log.NewContext(ctx, src.Logger)) if err != nil { return nil, err } @@ -103,7 +104,7 @@ func (s Source) ExportDevice(devEUI string) (*ttnpb.EndDevice, error) { Source: ttnpb.LocationSource_SOURCE_REGISTRY, }, } - logger.Debugw("Set location", "location", v3dev.Locations) + s.src.Logger.Debugw("Set location", "location", v3dev.Locations) } v3dev.Ids.DevEui, err = util.UnmarshalTextToBytes(&types.EUI64{}, ffdev.EUI) if err != nil { @@ -165,7 +166,7 @@ func (s Source) ExportDevice(devEUI string) (*ttnpb.EndDevice, error) { } if s.invalidateKeys { - logger.Debugw("Increment the last byte of the device keys", "device_id", ffdev.Name, "device_eui", ffdev.EUI) + s.src.Logger.Debugw("Increment the last byte of the device keys", "device_id", ffdev.Name, "device_eui", ffdev.EUI) // Increment the last byte of the device keys. // This makes it easier to rollback a migration if needed. updated := ffdev.WithIncrementedKeys() @@ -184,7 +185,7 @@ func (s Source) RangeDevices(_ string, f func(source.Source, string) error) erro devs []client.Device err error ) - logger.Debugw("Firefly LNS does not group devices by an application. Get all devices accessible by the API key") + s.src.Logger.Debugw("Firefly LNS does not group devices by an application. Get all devices accessible by the API key") devs, err = s.GetAllDevices() if err != nil { return err diff --git a/pkg/source/source.go b/pkg/source/source.go index 5da96f7..af1eccc 100644 --- a/pkg/source/source.go +++ b/pkg/source/source.go @@ -22,12 +22,15 @@ import ( "github.com/spf13/pflag" "go.thethings.network/lorawan-stack-migrate/pkg/iterator" "go.thethings.network/lorawan-stack/v3/pkg/ttnpb" + "go.uber.org/zap" ) type Config struct { DryRun, Verbose bool FrequencyPlansURL string + Logger *zap.SugaredLogger + source string } @@ -85,6 +88,15 @@ func NewSource(ctx context.Context) (Source, error) { if RootConfig.Source() == "" { return nil, ErrNoSource.New() } + cfg := zap.NewProductionConfig() + if RootConfig.Verbose { + cfg.Level = zap.NewAtomicLevelAt(zap.DebugLevel) + } + zapLogger, err := cfg.Build() + if err != nil { + return nil, err + } + RootConfig.Logger = zapLogger.Sugar() if registration, ok := registeredSources[RootConfig.Source()]; ok { return registration.Create(ctx, RootConfig) } diff --git a/pkg/source/tts/config/config.go b/pkg/source/tts/config/config.go index d8bd478..3e221eb 100644 --- a/pkg/source/tts/config/config.go +++ b/pkg/source/tts/config/config.go @@ -23,11 +23,8 @@ import ( "github.com/spf13/pflag" "go.thethings.network/lorawan-stack-migrate/pkg/source" "go.thethings.network/lorawan-stack-migrate/pkg/source/tts/api" - "go.uber.org/zap" ) -var logger *zap.SugaredLogger - type serverConfig struct { defaultGRPCAddress, ApplicationServerGRPCAddress, @@ -150,12 +147,6 @@ type Config struct { func (c *Config) Initialize(rootConfig source.Config) error { c.Config = rootConfig - var err error - logger, err = NewLogger(c.Verbose) - if err != nil { - return err - } - if c.appAPIKey == "" { return errNoAppAPIKey.New() } @@ -164,7 +155,7 @@ func (c *Config) Initialize(rootConfig source.Config) error { switch { case c.insecure: api.SetInsecure(true) - logger.Warn("Using insecure connection to API") + c.Logger.Warn("Using insecure connection to API") default: if c.caPath != "" { @@ -179,25 +170,13 @@ func (c *Config) Initialize(rootConfig source.Config) error { // DeleteSourceDevice is not allowed during a dry run if c.DryRun && c.DeleteSourceDevice { - logger.Warn("Cannot delete source devices during a dry run.") + c.Logger.Warn("Cannot delete source devices during a dry run.") c.DeleteSourceDevice = false } return nil } -func NewLogger(verbose bool) (*zap.SugaredLogger, error) { - cfg := zap.NewProductionConfig() - if verbose { - cfg.Level = zap.NewAtomicLevelAt(zap.DebugLevel) - } - logger, err := cfg.Build() - if err != nil { - return nil, err - } - return logger.Sugar(), nil -} - func setCustomCA(path string) error { pemBytes, err := os.ReadFile(path) if err != nil { diff --git a/pkg/source/tts/source.go b/pkg/source/tts/source.go index 1b70bd2..8258534 100644 --- a/pkg/source/tts/source.go +++ b/pkg/source/tts/source.go @@ -23,11 +23,8 @@ import ( "go.thethings.network/lorawan-stack-migrate/pkg/source/tts/api" "go.thethings.network/lorawan-stack-migrate/pkg/source/tts/config" "go.thethings.network/lorawan-stack/v3/pkg/ttnpb" - "go.uber.org/zap" ) -var logger *zap.SugaredLogger - // Source implements the Source interface. type Source struct { ctx context.Context @@ -173,7 +170,7 @@ func (s Source) getEndDevice(ids *ttnpb.EndDeviceIdentifiers, nsPaths, asPaths, res := &ttnpb.EndDevice{} if len(jsPaths) > 0 { if s.config.ServerConfig.JoinServerGRPCAddress == "" { - logger.With("paths", jsPaths).Warn("Join Server disabled but fields specified to get") + s.config.Logger.With("paths", jsPaths).Warn("Join Server disabled but fields specified to get") } else { js, err := api.Dial(s.ctx, s.config.ServerConfig.JoinServerGRPCAddress) if err != nil { @@ -184,7 +181,7 @@ func (s Source) getEndDevice(ids *ttnpb.EndDeviceIdentifiers, nsPaths, asPaths, FieldMask: ttnpb.FieldMask(jsPaths...), }) if err != nil { - logger.With("error", err).Warn("Could not get end device from Join Server") + s.config.Logger.With("error", err).Warn("Could not get end device from Join Server") } else { if err := validateDeviceIds(res.Ids, jsRes.Ids); err != nil { return nil, err @@ -198,7 +195,7 @@ func (s Source) getEndDevice(ids *ttnpb.EndDeviceIdentifiers, nsPaths, asPaths, } if len(asPaths) > 0 { if s.config.ServerConfig.ApplicationServerGRPCAddress == "" { - logger.With("paths", asPaths).Warn("Application Server disabled but fields specified to get") + s.config.Logger.With("paths", asPaths).Warn("Application Server disabled but fields specified to get") } else { as, err := api.Dial(s.ctx, s.config.ServerConfig.ApplicationServerGRPCAddress) if err != nil { @@ -222,7 +219,7 @@ func (s Source) getEndDevice(ids *ttnpb.EndDeviceIdentifiers, nsPaths, asPaths, } if len(nsPaths) > 0 { if s.config.ServerConfig.NetworkServerGRPCAddress == "" { - logger.With("paths", nsPaths).Warn("Network Server disabled but fields specified to get") + s.config.Logger.With("paths", nsPaths).Warn("Network Server disabled but fields specified to get") } else { ns, err := api.Dial(s.ctx, s.config.ServerConfig.NetworkServerGRPCAddress) if err != nil { @@ -260,7 +257,7 @@ func (s Source) setEndDevice(device *ttnpb.EndDevice, isPaths, nsPaths, asPaths, return nil, err } isDevice := &ttnpb.EndDevice{} - logger.With("paths", isPaths).Debug("Set end device on Identity Server") + s.config.Logger.With("paths", isPaths).Debug("Set end device on Identity Server") isDevice.SetFields(device, ttnpb.AddFields(ttnpb.ExcludeFields(isPaths, unsetPaths...), "ids")...) isRes, err := ttnpb.NewEndDeviceRegistryClient(is).Update(s.ctx, &ttnpb.UpdateEndDeviceRequest{ EndDevice: isDevice, @@ -276,14 +273,14 @@ func (s Source) setEndDevice(device *ttnpb.EndDevice, isPaths, nsPaths, asPaths, } if len(jsPaths) > 0 { if s.config.ServerConfig.JoinServerGRPCAddress == "" { - logger.With("paths", jsPaths).Warn("Join Server disabled but fields specified to set") + s.config.Logger.With("paths", jsPaths).Warn("Join Server disabled but fields specified to set") } else { js, err := api.Dial(s.ctx, s.config.ServerConfig.JoinServerGRPCAddress) if err != nil { return nil, err } jsDevice := &ttnpb.EndDevice{} - logger.With("paths", jsPaths).Debug("Set end device on Join Server") + s.config.Logger.With("paths", jsPaths).Debug("Set end device on Join Server") if err := jsDevice.SetFields(device, ttnpb.AddFields(ttnpb.ExcludeFields(jsPaths, unsetPaths...), "ids")...); err != nil { return nil, err } @@ -302,14 +299,14 @@ func (s Source) setEndDevice(device *ttnpb.EndDevice, isPaths, nsPaths, asPaths, } if len(nsPaths) > 0 { if s.config.ServerConfig.NetworkServerGRPCAddress == "" { - logger.With("paths", nsPaths).Warn("Network Server disabled but fields specified to set") + s.config.Logger.With("paths", nsPaths).Warn("Network Server disabled but fields specified to set") } else { ns, err := api.Dial(s.ctx, s.config.ServerConfig.NetworkServerGRPCAddress) if err != nil { return nil, err } nsDevice := &ttnpb.EndDevice{} - logger.With("paths", nsPaths).Debug("Set end device on Network Server") + s.config.Logger.With("paths", nsPaths).Debug("Set end device on Network Server") if err := nsDevice.SetFields(device, ttnpb.AddFields(ttnpb.ExcludeFields(nsPaths, unsetPaths...), "ids")...); err != nil { return nil, err } @@ -328,14 +325,14 @@ func (s Source) setEndDevice(device *ttnpb.EndDevice, isPaths, nsPaths, asPaths, } if len(asPaths) > 0 { if s.config.ServerConfig.ApplicationServerGRPCAddress == "" { - logger.With("paths", asPaths).Warn("Application Server disabled but fields specified to set") + s.config.Logger.With("paths", asPaths).Warn("Application Server disabled but fields specified to set") } else { as, err := api.Dial(s.ctx, s.config.ServerConfig.ApplicationServerGRPCAddress) if err != nil { return nil, err } asDevice := &ttnpb.EndDevice{} - logger.With("paths", asPaths).Debug("Set end device on Application Server") + s.config.Logger.With("paths", asPaths).Debug("Set end device on Application Server") if err := asDevice.SetFields(device, ttnpb.AddFields(ttnpb.ExcludeFields(asPaths, unsetPaths...), "ids")...); err != nil { return nil, err } diff --git a/pkg/source/tts/tts.go b/pkg/source/tts/tts.go index 8e12086..b6b756d 100644 --- a/pkg/source/tts/tts.go +++ b/pkg/source/tts/tts.go @@ -22,8 +22,6 @@ import ( func init() { cfg, flags := config.New() - logger, _ = config.NewLogger(cfg.Verbose) - source.RegisterSource(source.Registration{ Name: "tts", Description: "Migrate from The Things Stack",