Ignore sql.ErrNoRows

[vutung2311]

There are some cases that gorm can return sql.ErrNoRows when using with postgres and ON CONFLICT DO NOTHING RETURNING *. I think we can safely ignore this error as we're ignoring gorm.ErrRecordNotFound

Also add second argument of redis command trace to understand more clearly about the trace.

[codecov-io]

Codecov Report

Merging #645 into master will decrease coverage by 0.46%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #645      +/-   ##
=========================================
- Coverage   88.36%   87.9%   -0.47%     
=========================================
  Files         123     123              
  Lines        7900    7640     -260     
=========================================
- Hits         6981    6716     -265     
- Misses        798     821      +23     
+ Partials      121     103      -18

Impacted Files	Coverage Δ
module/apmgorm/context.go	88.57% <100%> (ø)	⬆️
model/marshal_fastjson.go	77.1% <0%> (-5.58%)	⬇️
utils.go	82.69% <0%> (-1.18%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b7289b...4f0ee11. Read the comment docs.

[axw]

@vutung2311 thanks for the PR!

The Gorm change seems reasonable, but can you please add a minimal test that fails before the change?

The Redis change should be made separately, if at all - please remove that from this PR so we can focus on one change at a time. If we do make the Redis change, it should be made across all of the agents - so please open a discussion at https://discuss.elastic.co/c/apm first.

[vutung2311]

@vutung2311 thanks for the PR!

The Gorm change seems reasonable, but can you please add a minimal test that fails before the change?

The Redis change should be made separately, if at all - please remove that from this PR so we can focus on one change at a time. If we do make the Redis change, it should be made across all of the agents - so please open a discussion at https://discuss.elastic.co/c/apm first.

Understood. I'm gonna work on it.

[vutung2311]

I tried quite a few ways for sqlite3 but making that test to have sql.ErrNoRows was not possible for me. I modified the test to use Postgres. I hope that won't hurt anything.

[axw]

I tried quite a few ways for sqlite3 but making that test to have sql.ErrNoRows was not possible for me. I modified the test to use Postgres. I hope that won't hurt anything.

@vutung2311 thanks for the updates. I'd like to keep running the test against sqlite3, so it can be run without starting up PostgreSQL. How about this?

func TestCaptureErrors(t *testing.T) {
        t.Run("sqlite3", func(t *testing.T) {
                db, err := apmgorm.Open("sqlite3", ":memory:")
                require.NoError(t, err)
                defer db.Close()
        })

        if os.Getenv("PGHOST") == "" {
                t.Logf("PGHOST not specified, skipping")
        } else {
                t.Run("postgres", func(t *testing.T) {
                        db, err := apmgorm.Open("postgres", "user=postgres password=hunter2 dbname=test_db sslmode=disable")
                        require.NoError(t, err)
                        defer db.Close()
                })
        }
}

func testCaptureErrors(t *testing.T, db *gorm.DB, f func(ctx context.Context, db *gorm.DB)) {
        db.SetLogger(nopLogger{})
        db.AutoMigrate(&Product{})
        require.NoError(t, db.Unscoped().Delete(&Product{}).Error)

        _, spans, errors := apmtest.WithTransaction(func(ctx context.Context) {
                db = apmgorm.WithContext(ctx, db)

                // gorm.ErrRecordNotFound should not cause an error
                db.Where("code=?", "L1212").First(&Product{})

                // The postgres dialect will use QueryRow when inserting, in order to fetch
                // the inserted row ID. By using "ON CONFLICT (id) DO NOTHING", no row will
                // be inserted, and QueryRow will return sql.ErrNoRows. This should not be
                // reported as an error.
                product := Product{
                        Model: gorm.Model{
                                ID: 1001,
                        },
                        Code:  "1001",
                        Price: 1001,
                }
                require.NoError(t, db.Create(&product).Error)
                db.Set("gorm:insert_option", "ON CONFLICT (id) DO NOTHING").Create(&product)

                // invalid SQL should
                db.Where("bananas").First(&Product{})
        })
        assert.Len(t, spans, 4)
        require.Len(t, errors, 1)
        assert.Regexp(t, `.*bananas.*`, errors[0].Exception.Message)
}

[vutung2311]

I tried quite a few ways for sqlite3 but making that test to have sql.ErrNoRows was not possible for me. I modified the test to use Postgres. I hope that won't hurt anything.

@vutung2311 thanks for the updates. I'd like to keep running the test against sqlite3, so it can be run without starting up PostgreSQL. How about this?

func TestCaptureErrors(t *testing.T) {
        t.Run("sqlite3", func(t *testing.T) {
                db, err := apmgorm.Open("sqlite3", ":memory:")
                require.NoError(t, err)
                defer db.Close()
        })

        if os.Getenv("PGHOST") == "" {
                t.Logf("PGHOST not specified, skipping")
        } else {
                t.Run("postgres", func(t *testing.T) {
                        db, err := apmgorm.Open("postgres", "user=postgres password=hunter2 dbname=test_db sslmode=disable")
                        require.NoError(t, err)
                        defer db.Close()
                })
        }
}

func testCaptureErrors(t *testing.T, db *gorm.DB, f func(ctx context.Context, db *gorm.DB)) {
        db.SetLogger(nopLogger{})
        db.AutoMigrate(&Product{})
        require.NoError(t, db.Unscoped().Delete(&Product{}).Error)

        _, spans, errors := apmtest.WithTransaction(func(ctx context.Context) {
                db = apmgorm.WithContext(ctx, db)

                // gorm.ErrRecordNotFound should not cause an error
                db.Where("code=?", "L1212").First(&Product{})

                // The postgres dialect will use QueryRow when inserting, in order to fetch
                // the inserted row ID. By using "ON CONFLICT (id) DO NOTHING", no row will
                // be inserted, and QueryRow will return sql.ErrNoRows. This should not be
                // reported as an error.
                product := Product{
                        Model: gorm.Model{
                                ID: 1001,
                        },
                        Code:  "1001",
                        Price: 1001,
                }
                require.NoError(t, db.Create(&product).Error)
                db.Set("gorm:insert_option", "ON CONFLICT (id) DO NOTHING").Create(&product)

                // invalid SQL should
                db.Where("bananas").First(&Product{})
        })
        assert.Len(t, spans, 4)
        require.Len(t, errors, 1)
        assert.Regexp(t, `.*bananas.*`, errors[0].Exception.Message)
}

Thank you for your suggestion. I changed the file with few modifications to make sure that tests are independent of each others.

[axw]

Thanks @vutung2311, looks good! Just a couple of minor issues left, then it should be good to merge.

[axw]

jenkins, run the tests please

[axw]

Thanks again for your contribution @vutung2311!