From c808b17567d77d6dd1c27d477bf82985c3dcc7f4 Mon Sep 17 00:00:00 2001 From: Victor Lellis Date: Sat, 29 May 2021 23:20:50 -0300 Subject: [PATCH 1/4] gorm.io/gorm.v1: add err check --- contrib/gorm.io/gorm.v1/gorm.go | 5 ++- contrib/gorm.io/gorm.v1/gorm_test.go | 50 ++++++++++++++++++++++++++++ contrib/gorm.io/gorm.v1/option.go | 11 ++++++ 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/contrib/gorm.io/gorm.v1/gorm.go b/contrib/gorm.io/gorm.v1/gorm.go index 41a60f2e19..2dda183cca 100644 --- a/contrib/gorm.io/gorm.v1/gorm.go +++ b/contrib/gorm.io/gorm.v1/gorm.go @@ -121,5 +121,8 @@ func after(db *gorm.DB, operationName string, cfg *config) { } span, _ := tracer.StartSpanFromContext(ctx, operationName, opts...) - span.Finish(tracer.WithError(db.Error)) + defer span.Finish() + if cfg.errCheck(db.Error) { + span.SetTag(ext.Error, db.Error) + } } diff --git a/contrib/gorm.io/gorm.v1/gorm_test.go b/contrib/gorm.io/gorm.v1/gorm_test.go index 080ff861cd..5f8db73580 100644 --- a/contrib/gorm.io/gorm.v1/gorm_test.go +++ b/contrib/gorm.io/gorm.v1/gorm_test.go @@ -343,3 +343,53 @@ func TestContext(t *testing.T) { assert.Equal(t, testCtx.Value(key(contextKey)), ctx.Value(key(contextKey))) }) } + +func TestError(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + assertErrCheck := func(t *testing.T, mt mocktracer.Tracer, errExist bool, opts ...Option) { + sqltrace.Register("pgx", &stdlib.Driver{}) + sqlDb, err := sqltrace.Open("pgx", pgConnString) + if err != nil { + log.Fatal(err) + } + + db, err := Open(postgres.New(postgres.Config{Conn: sqlDb}), &gorm.Config{}) + if err != nil { + log.Fatal(err) + } + + db.AutoMigrate(&Product{}) + + db = db.WithContext(context.Background()) + db.Find(&Product{}, Product{Code: "L1210", Price: 2000}) + + spans := mt.FinishedSpans() + assert.True(t, len(spans) > 1) + + // Get last span (gorm.db) + s := spans[len(spans)-1] + assert.Equal(t, errExist, s.Tag(ext.Error) != nil) + } + + t.Run("defaults", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + assertErrCheck(t, mt, true) + }) + + t.Run("errcheck", func(t *testing.T) { + mt := mocktracer.Start() + defer mt.Stop() + + errFn := func(err error) bool { + if err == gorm.ErrRecordNotFound { + return false + } + return true + } + assertErrCheck(t, mt, false, WithErrorCheck(errFn)) + }) +} diff --git a/contrib/gorm.io/gorm.v1/option.go b/contrib/gorm.io/gorm.v1/option.go index 89bc9cbec9..a15ed399cf 100644 --- a/contrib/gorm.io/gorm.v1/option.go +++ b/contrib/gorm.io/gorm.v1/option.go @@ -15,6 +15,7 @@ type config struct { serviceName string analyticsRate float64 dsn string + errCheck func(err error) bool } // Option represents an option that can be passed to Register, Open or OpenDB. @@ -28,6 +29,7 @@ func defaults(cfg *config) { } else { cfg.analyticsRate = math.NaN() } + cfg.errCheck = func(error) bool { return true } } // WithServiceName sets the given service name when registering a driver, @@ -60,3 +62,12 @@ func WithAnalyticsRate(rate float64) Option { } } } + +// WithErrorCheck specifies a function fn which determines whether the passed +// error should be marked as an error. The fn is called whenever a gorm operation +// finishes with an error +func WithErrorCheck(fn func(err error) bool) Option { + return func(cfg *config) { + cfg.errCheck = fn + } +} From 6b8910f4f757ffd2e1c1d5beb2076b1e457e4dd3 Mon Sep 17 00:00:00 2001 From: Victor Lellis Date: Fri, 3 Sep 2021 20:05:08 -0300 Subject: [PATCH 2/4] gorm.io/gorm.v1: fix test case --- contrib/gorm.io/gorm.v1/gorm.go | 5 +++-- contrib/gorm.io/gorm.v1/gorm_test.go | 21 ++++++--------------- 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/contrib/gorm.io/gorm.v1/gorm.go b/contrib/gorm.io/gorm.v1/gorm.go index 2dda183cca..3752716a24 100644 --- a/contrib/gorm.io/gorm.v1/gorm.go +++ b/contrib/gorm.io/gorm.v1/gorm.go @@ -121,8 +121,9 @@ func after(db *gorm.DB, operationName string, cfg *config) { } span, _ := tracer.StartSpanFromContext(ctx, operationName, opts...) - defer span.Finish() + var dbErr error if cfg.errCheck(db.Error) { - span.SetTag(ext.Error, db.Error) + dbErr = db.Error } + span.Finish(tracer.WithError(dbErr)) } diff --git a/contrib/gorm.io/gorm.v1/gorm_test.go b/contrib/gorm.io/gorm.v1/gorm_test.go index 5f8db73580..c147b6d506 100644 --- a/contrib/gorm.io/gorm.v1/gorm_test.go +++ b/contrib/gorm.io/gorm.v1/gorm_test.go @@ -351,25 +351,19 @@ func TestError(t *testing.T) { assertErrCheck := func(t *testing.T, mt mocktracer.Tracer, errExist bool, opts ...Option) { sqltrace.Register("pgx", &stdlib.Driver{}) sqlDb, err := sqltrace.Open("pgx", pgConnString) - if err != nil { - log.Fatal(err) - } - - db, err := Open(postgres.New(postgres.Config{Conn: sqlDb}), &gorm.Config{}) - if err != nil { - log.Fatal(err) - } + assert.Nil(t, err) + db, err := Open(postgres.New(postgres.Config{Conn: sqlDb}), &gorm.Config{}, opts...) + assert.Nil(t, err) db.AutoMigrate(&Product{}) - - db = db.WithContext(context.Background()) - db.Find(&Product{}, Product{Code: "L1210", Price: 2000}) + db.First(&Product{}, Product{Code: "L1210", Price: 2000}) spans := mt.FinishedSpans() assert.True(t, len(spans) > 1) // Get last span (gorm.db) s := spans[len(spans)-1] + assert.Equal(t, errExist, s.Tag(ext.Error) != nil) } @@ -385,10 +379,7 @@ func TestError(t *testing.T) { defer mt.Stop() errFn := func(err error) bool { - if err == gorm.ErrRecordNotFound { - return false - } - return true + return err != gorm.ErrRecordNotFound } assertErrCheck(t, mt, false, WithErrorCheck(errFn)) }) From d8ea4f2e4faf6421f037f6841d8c7da15a169c93 Mon Sep 17 00:00:00 2001 From: Victor Lellis Date: Fri, 3 Sep 2021 20:07:52 -0300 Subject: [PATCH 3/4] gorm.io/gorm.v1: delete blank lines --- contrib/gorm.io/gorm.v1/gorm_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/contrib/gorm.io/gorm.v1/gorm_test.go b/contrib/gorm.io/gorm.v1/gorm_test.go index c147b6d506..9d4d1fec19 100644 --- a/contrib/gorm.io/gorm.v1/gorm_test.go +++ b/contrib/gorm.io/gorm.v1/gorm_test.go @@ -370,14 +370,12 @@ func TestError(t *testing.T) { t.Run("defaults", func(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() - assertErrCheck(t, mt, true) }) t.Run("errcheck", func(t *testing.T) { mt := mocktracer.Start() defer mt.Stop() - errFn := func(err error) bool { return err != gorm.ErrRecordNotFound } From 1505f4b48ae60c0f4e0b6f1e473afde29d786fd3 Mon Sep 17 00:00:00 2001 From: Victor Lellis Date: Wed, 13 Oct 2021 12:53:53 -0300 Subject: [PATCH 4/4] gorm.io/gorm.v1: Update comment Co-authored-by: Kyle Nusbaum --- contrib/gorm.io/gorm.v1/option.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/gorm.io/gorm.v1/option.go b/contrib/gorm.io/gorm.v1/option.go index a15ed399cf..6dfa9b2944 100644 --- a/contrib/gorm.io/gorm.v1/option.go +++ b/contrib/gorm.io/gorm.v1/option.go @@ -65,7 +65,7 @@ func WithAnalyticsRate(rate float64) Option { // WithErrorCheck specifies a function fn which determines whether the passed // error should be marked as an error. The fn is called whenever a gorm operation -// finishes with an error +// finishes func WithErrorCheck(fn func(err error) bool) Option { return func(cfg *config) { cfg.errCheck = fn