From 4e531d452451e04639f59a639c71ea6c81233b4c Mon Sep 17 00:00:00 2001 From: Michal Horejsek Date: Wed, 10 Feb 2021 10:36:37 +0100 Subject: [PATCH] GODT-1036 Event loop Sentry reporting of failures and refresh --- go.mod | 1 + go.sum | 2 ++ internal/app/base/base.go | 54 +++++++++++++++++--------------- internal/app/bridge/bridge.go | 3 +- internal/bridge/bridge.go | 4 ++- internal/bridge/store_factory.go | 27 +++++++++------- internal/store/event_loop.go | 25 +++++++++++++-- internal/store/store.go | 30 ++++++++++-------- internal/store/store_test.go | 1 + internal/users/users_test.go | 4 ++- test/context/bridge.go | 5 ++- 11 files changed, 97 insertions(+), 59 deletions(-) diff --git a/go.mod b/go.mod index 621dfbe4..f3ec37b7 100644 --- a/go.mod +++ b/go.mod @@ -54,6 +54,7 @@ require ( github.com/nsf/jsondiff v0.0.0-20200515183724-f29ed568f4ce github.com/olekukonko/tablewriter v0.0.4 // indirect github.com/pkg/errors v0.9.1 + github.com/pkg/math v0.0.0-20141027224758-f2ed9e40e245 github.com/sirupsen/logrus v1.7.0 github.com/skratchdot/open-golang v0.0.0-20200116055534-eef842397966 github.com/ssor/bom v0.0.0-20170718123548-6386211fdfcf // indirect diff --git a/go.sum b/go.sum index f96c74dd..38be23b8 100644 --- a/go.sum +++ b/go.sum @@ -223,6 +223,8 @@ github.com/pingcap/errors v0.11.4/go.mod h1:Oi8TUi2kEtXXLMJk9l1cGmz20kV3TaQ0usTw github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pkg/math v0.0.0-20141027224758-f2ed9e40e245 h1:gk/AF9SGRj+RafNCoDcS3RRscb8S4BVbvqODOgWA7/8= +github.com/pkg/math v0.0.0-20141027224758-f2ed9e40e245/go.mod h1:2dhPPj2Li3DXrSY2U2ADdZy2B7sjQsT57lqENx1+FSE= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday v1.5.2 h1:HyvC0ARfnZBqnXwABFeSZHpKvJHJJfPz81GNueLj0oo= diff --git a/internal/app/base/base.go b/internal/app/base/base.go index 65893546..bfcdfabd 100644 --- a/internal/app/base/base.go +++ b/internal/app/base/base.go @@ -76,19 +76,20 @@ const ( ) type Base struct { - CrashHandler *crash.Handler - Locations *locations.Locations - Settings *settings.Settings - Lock *os.File - Cache *cache.Cache - Listener listener.Listener - Creds *credentials.Store - CM *pmapi.ClientManager - CookieJar *cookies.Jar - Updater *updater.Updater - Versioner *versioner.Versioner - TLS *tls.TLS - Autostart *autostart.App + SentryReporter *sentry.Reporter + CrashHandler *crash.Handler + Locations *locations.Locations + Settings *settings.Settings + Lock *os.File + Cache *cache.Cache + Listener listener.Listener + Creds *credentials.Store + CM *pmapi.ClientManager + CookieJar *cookies.Jar + Updater *updater.Updater + Versioner *versioner.Versioner + TLS *tls.TLS + Autostart *autostart.App Name string // the app's name usage string // the app's usage description @@ -234,19 +235,20 @@ func New( // nolint[funlen] } return &Base{ - CrashHandler: crashHandler, - Locations: locations, - Settings: settingsObj, - Lock: lock, - Cache: cache, - Listener: listener, - Creds: credentials.NewStore(kc), - CM: cm, - CookieJar: jar, - Updater: updater, - Versioner: versioner, - TLS: tls.New(settingsPath), - Autostart: autostart, + SentryReporter: sentryReporter, + CrashHandler: crashHandler, + Locations: locations, + Settings: settingsObj, + Lock: lock, + Cache: cache, + Listener: listener, + Creds: credentials.NewStore(kc), + CM: cm, + CookieJar: jar, + Updater: updater, + Versioner: versioner, + TLS: tls.New(settingsPath), + Autostart: autostart, Name: appName, usage: appUsage, diff --git a/internal/app/bridge/bridge.go b/internal/app/bridge/bridge.go index 678cdb4c..54544254 100644 --- a/internal/app/bridge/bridge.go +++ b/internal/app/bridge/bridge.go @@ -71,8 +71,7 @@ func run(b *base.Base, c *cli.Context) error { // nolint[funlen] if err != nil { logrus.WithError(err).Fatal("Failed to load TLS config") } - - bridge := bridge.New(b.Locations, b.Cache, b.Settings, b.CrashHandler, b.Listener, b.CM, b.Creds, b.Updater, b.Versioner) + bridge := bridge.New(b.Locations, b.Cache, b.Settings, b.SentryReporter, b.CrashHandler, b.Listener, b.CM, b.Creds, b.Updater, b.Versioner) imapBackend := imap.NewIMAPBackend(b.CrashHandler, b.Listener, b.Cache, bridge) smtpBackend := smtp.NewSMTPBackend(b.CrashHandler, b.Listener, b.Settings, bridge) diff --git a/internal/bridge/bridge.go b/internal/bridge/bridge.go index e9a40e6f..b5249315 100644 --- a/internal/bridge/bridge.go +++ b/internal/bridge/bridge.go @@ -29,6 +29,7 @@ import ( "github.com/ProtonMail/proton-bridge/internal/updater" "github.com/ProtonMail/proton-bridge/internal/users" "github.com/ProtonMail/proton-bridge/pkg/pmapi" + "github.com/ProtonMail/proton-bridge/pkg/sentry" "github.com/ProtonMail/proton-bridge/pkg/listener" logrus "github.com/sirupsen/logrus" @@ -56,6 +57,7 @@ func New( locations Locator, cache Cacher, s SettingsProvider, + sentryReporter *sentry.Reporter, panicHandler users.PanicHandler, eventListener listener.Listener, clientManager users.ClientManager, @@ -69,7 +71,7 @@ func New( clientManager.AllowProxy() } - storeFactory := newStoreFactory(cache, panicHandler, clientManager, eventListener) + storeFactory := newStoreFactory(cache, sentryReporter, panicHandler, clientManager, eventListener) u := users.New(locations, panicHandler, eventListener, clientManager, credStorer, storeFactory, true) b := &Bridge{ Users: u, diff --git a/internal/bridge/store_factory.go b/internal/bridge/store_factory.go index febe3af1..ba15b870 100644 --- a/internal/bridge/store_factory.go +++ b/internal/bridge/store_factory.go @@ -23,37 +23,40 @@ import ( "github.com/ProtonMail/proton-bridge/internal/store" "github.com/ProtonMail/proton-bridge/internal/users" - "github.com/ProtonMail/proton-bridge/pkg/listener" + "github.com/ProtonMail/proton-bridge/pkg/sentry" ) type storeFactory struct { - cache Cacher - panicHandler users.PanicHandler - clientManager users.ClientManager - eventListener listener.Listener - storeCache *store.Cache + cache Cacher + sentryReporter *sentry.Reporter + panicHandler users.PanicHandler + clientManager users.ClientManager + eventListener listener.Listener + storeCache *store.Cache } func newStoreFactory( cache Cacher, + sentryReporter *sentry.Reporter, panicHandler users.PanicHandler, clientManager users.ClientManager, eventListener listener.Listener, ) *storeFactory { return &storeFactory{ - cache: cache, - panicHandler: panicHandler, - clientManager: clientManager, - eventListener: eventListener, - storeCache: store.NewCache(cache.GetIMAPCachePath()), + cache: cache, + sentryReporter: sentryReporter, + panicHandler: panicHandler, + clientManager: clientManager, + eventListener: eventListener, + storeCache: store.NewCache(cache.GetIMAPCachePath()), } } // New creates new store for given user. func (f *storeFactory) New(user store.BridgeUser) (*store.Store, error) { storePath := getUserStorePath(f.cache.GetDBDir(), user.ID()) - return store.New(f.panicHandler, user, f.clientManager, f.eventListener, storePath, f.storeCache) + return store.New(f.sentryReporter, f.panicHandler, user, f.clientManager, f.eventListener, storePath, f.storeCache) } // Remove removes all store files for given user. diff --git a/internal/store/event_loop.go b/internal/store/event_loop.go index afd0e01c..bdecfd91 100644 --- a/internal/store/event_loop.go +++ b/internal/store/event_loop.go @@ -28,8 +28,13 @@ import ( "github.com/sirupsen/logrus" ) -const pollInterval = 30 * time.Second -const pollIntervalSpread = 5 * time.Second +const ( + pollInterval = 30 * time.Second + pollIntervalSpread = 5 * time.Second + + // errMaxSentry defines after how many errors in a row to report it to sentry. + errMaxSentry = 20 +) type eventLoop struct { cache *Cache @@ -41,6 +46,7 @@ type eventLoop struct { isRunning bool // The whole event loop is running. pollCounter int + errCounter int log *logrus.Entry @@ -227,9 +233,18 @@ func (loop *eventLoop) processNextEvent() (more bool, err error) { // nolint[fun _, errUnauthorized := errors.Cause(err).(*pmapi.ErrUnauthorized) + if err == nil { + loop.errCounter = 0 + } // All errors except Invalid Token (which is not possible to recover from) are ignored. if err != nil && !errUnauthorized && errors.Cause(err) != pmapi.ErrInvalidToken { - l.WithError(err).Error("Error skipped") + l.WithError(err).WithField("errors", loop.errCounter).Error("Error skipped") + loop.errCounter++ + if loop.errCounter == errMaxSentry { + if sentryErr := loop.store.sentryReporter.ReportMessage("Warning: event loop issues: " + err.Error() + ", " + loop.currentEventID); sentryErr != nil { + l.WithError(sentryErr).Error("Failed to report error to sentry") + } + } err = nil } }() @@ -283,6 +298,10 @@ func (loop *eventLoop) processEvent(event *pmapi.Event) (err error) { eventLog.Info("Processing refresh event") loop.store.triggerSync() + if sentryErr := loop.store.sentryReporter.ReportMessage("Warning: refresh occurred, " + loop.currentEventID); sentryErr != nil { + loop.log.WithError(sentryErr).Error("Failed to report refresh to sentry") + } + return } diff --git a/internal/store/store.go b/internal/store/store.go index cacd9201..6f7482f0 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -26,6 +26,7 @@ import ( "github.com/ProtonMail/proton-bridge/pkg/listener" "github.com/ProtonMail/proton-bridge/pkg/pmapi" + "github.com/ProtonMail/proton-bridge/pkg/sentry" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -95,10 +96,11 @@ var ( // Store is local user storage, which handles the synchronization between IMAP and PM API. type Store struct { - panicHandler PanicHandler - eventLoop *eventLoop - user BridgeUser - clientManager ClientManager + sentryReporter *sentry.Reporter + panicHandler PanicHandler + eventLoop *eventLoop + user BridgeUser + clientManager ClientManager log *logrus.Entry @@ -115,7 +117,8 @@ type Store struct { } // New creates or opens a store for the given `user`. -func New( +func New( // nolint[funlen] + sentryReporter *sentry.Reporter, panicHandler PanicHandler, user BridgeUser, clientManager ClientManager, @@ -145,14 +148,15 @@ func New( } store = &Store{ - panicHandler: panicHandler, - clientManager: clientManager, - user: user, - cache: cache, - filePath: path, - db: bdb, - lock: &sync.RWMutex{}, - log: l, + sentryReporter: sentryReporter, + panicHandler: panicHandler, + clientManager: clientManager, + user: user, + cache: cache, + filePath: path, + db: bdb, + lock: &sync.RWMutex{}, + log: l, } // Minimal increase is event pollInterval, doubles every failed retry up to 5 minutes. diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 0f2aafe5..712ccd60 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -125,6 +125,7 @@ func (mocks *mocksForStore) newStoreNoEvents(combinedMode bool, msgs ...*pmapi.M var err error mocks.store, err = New( + nil, // Sentry reporter is not used under unit tests. mocks.panicHandler, mocks.user, mocks.clientManager, diff --git a/internal/users/users_test.go b/internal/users/users_test.go index baa92d05..f75289ad 100644 --- a/internal/users/users_test.go +++ b/internal/users/users_test.go @@ -31,6 +31,7 @@ import ( usersmocks "github.com/ProtonMail/proton-bridge/internal/users/mocks" "github.com/ProtonMail/proton-bridge/pkg/pmapi" pmapimocks "github.com/ProtonMail/proton-bridge/pkg/pmapi/mocks" + "github.com/ProtonMail/proton-bridge/pkg/sentry" gomock "github.com/golang/mock/gomock" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" @@ -185,9 +186,10 @@ func initMocks(t *testing.T) mocks { // Set up store factory. m.storeMaker.EXPECT().New(gomock.Any()).DoAndReturn(func(user store.BridgeUser) (*store.Store, error) { + var sentryReporter *sentry.Reporter // Sentry reporter is not used under unit tests. dbFile, err := ioutil.TempFile("", "bridge-store-db-*.db") require.NoError(t, err, "could not get temporary file for store db") - return store.New(m.PanicHandler, user, m.clientManager, m.eventListener, dbFile.Name(), m.storeCache) + return store.New(sentryReporter, m.PanicHandler, user, m.clientManager, m.eventListener, dbFile.Name(), m.storeCache) }).AnyTimes() m.storeMaker.EXPECT().Remove(gomock.Any()).AnyTimes() diff --git a/test/context/bridge.go b/test/context/bridge.go index 6664b12e..c3878e24 100644 --- a/test/context/bridge.go +++ b/test/context/bridge.go @@ -21,8 +21,10 @@ import ( "time" "github.com/ProtonMail/proton-bridge/internal/bridge" + "github.com/ProtonMail/proton-bridge/internal/constants" "github.com/ProtonMail/proton-bridge/internal/users" "github.com/ProtonMail/proton-bridge/pkg/listener" + "github.com/ProtonMail/proton-bridge/pkg/sentry" ) // GetBridge returns bridge instance. @@ -67,8 +69,9 @@ func newBridgeInstance( eventListener listener.Listener, clientManager users.ClientManager, ) *bridge.Bridge { + sentryReporter := sentry.NewReporter("bridge", constants.Version) panicHandler := &panicHandler{t: t} updater := newFakeUpdater() versioner := newFakeVersioner() - return bridge.New(locations, cache, settings, panicHandler, eventListener, clientManager, credStore, updater, versioner) + return bridge.New(locations, cache, settings, sentryReporter, panicHandler, eventListener, clientManager, credStore, updater, versioner) }