Added logging to tx rollback failure

Added few comments to clarify commit/rollback flow

Author: Fiery-Fenix

extension/storage/dbstorage/client.go

@@ -14,6 +14,7 @@ import (
 	// SQLite driver
 	_ "github.com/mattn/go-sqlite3"
 	"go.opentelemetry.io/collector/extension/xextension/storage"
+	"go.uber.org/zap"
 )
 
 const (
@@ -25,15 +26,16 @@ const (
 )
 
 type dbStorageClient struct {
+	logger      *zap.Logger
 	db          *sql.DB
 	getQuery    *sql.Stmt
 	setQuery    *sql.Stmt
 	deleteQuery *sql.Stmt
 }
 
-func newClient(ctx context.Context, driverName string, db *sql.DB, tableName string) (*dbStorageClient, error) {
+func newClient(ctx context.Context, logger *zap.Logger, db *sql.DB, driverName string, tableName string) (*dbStorageClient, error) {
 	createTableSQL := createTable
-	if driverName == driverSqlite {
+	if driverName == driverSQLite {
 		createTableSQL = createTableSqlite
 	}
 	var err error
@@ -54,7 +56,7 @@ func newClient(ctx context.Context, driverName string, db *sql.DB, tableName str
 	if err != nil {
 		return nil, err
 	}
-	return &dbStorageClient{db, selectQuery, setQuery, deleteQuery}, nil
+	return &dbStorageClient{logger, db, selectQuery, setQuery, deleteQuery}, nil
 }
 
 // Get will retrieve data from storage that corresponds to the specified key
@@ -79,8 +81,18 @@ func (c *dbStorageClient) Batch(ctx context.Context, ops ...*storage.Operation)
 	if err != nil {
 		return err
 	}
-	//nolint:errcheck
-	defer tx.Rollback()
+
+	// In case of any error we should roll back whole transaction to keep DB in consistent state
+	// In case of successful commit - tx.Rollback() will be a no-op here as tx is already closed
+	defer func() {
+		// We should ignore error related already finished transaction here
+		// It might happened, for example, if Context was canceled outside of Batch() function
+		// in this case whole transaction will be rolled back by sql package and we'll receive ErrTxDone here,
+		// which is actually not an issue because transaction was correctly closed with rollback
+		if rollbackErr := tx.Rollback(); !errors.Is(rollbackErr, sql.ErrTxDone) {
+			c.logger.Error("Failed to rollback Batch() transaction", zap.Error(rollbackErr))
+		}
+	}()
 
 	for _, op := range ops {
 		switch op.Type {

extension/storage/dbstorage/client_test.go

@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.opentelemetry.io/collector/extension/xextension/storage"
+	"go.uber.org/zap"
 )
 
 var testTableName = "exporter_otlp_test"
@@ -30,7 +31,7 @@ func Test_newClient(t *testing.T) {
 		mock.ExpectPrepare(regexp.QuoteMeta(fmt.Sprintf(setQueryText, testTableName)))
 		mock.ExpectPrepare(regexp.QuoteMeta(fmt.Sprintf(deleteQueryText, testTableName)))
 
-		_, err = newClient(context.Background(), driverPostgresql, db, testTableName)
+		_, err = newClient(context.Background(), zap.L(), db, driverPostgreSQL, testTableName)
 		assert.NoError(t, err)
 	})
 	t.Run("Should return client with Sqlite specific query(s)", func(t *testing.T) {
@@ -44,7 +45,7 @@ func Test_newClient(t *testing.T) {
 		mock.ExpectPrepare(regexp.QuoteMeta(fmt.Sprintf(setQueryText, testTableName)))
 		mock.ExpectPrepare(regexp.QuoteMeta(fmt.Sprintf(deleteQueryText, testTableName)))
 
-		_, err = newClient(context.Background(), driverSqlite, db, testTableName)
+		_, err = newClient(context.Background(), zap.L(), db, driverSQLite, testTableName)
 		assert.NoError(t, err)
 	})
 }
@@ -277,6 +278,7 @@ func newTestClient(t *testing.T) (*dbStorageClient, sqlmock.Sqlmock) {
 	require.NoError(t, err)
 
 	return &dbStorageClient{
+		logger:      zap.L(),
 		db:          db,
 		getQuery:    selectQuery,
 		setQuery:    setQuery,

extension/storage/dbstorage/config.go

@@ -9,8 +9,8 @@ import (
 )
 
 const (
-	driverPostgresql = "pgx"
-	driverSqlite     = "sqlite3"
+	driverPostgreSQL = "pgx"
+	driverSQLite     = "sqlite3"
 )
 
 // Config defines configuration for dbstorage extension.
@@ -27,7 +27,7 @@ func (cfg *Config) Validate() error {
 		return errors.New("missing driver name")
 	}
 
-	if cfg.DriverName != driverPostgresql && cfg.DriverName != driverSqlite {
+	if cfg.DriverName != driverPostgreSQL && cfg.DriverName != driverSQLite {
 		return fmt.Errorf("unsupported driver %s", cfg.DriverName)
 	}
 

extension/storage/dbstorage/config_test.go

@@ -34,7 +34,7 @@ func TestConfigValidate(t *testing.T) {
 		},
 		{
 			"Valid",
-			Config{DriverName: driverSqlite, DataSource: "bar"},
+			Config{DriverName: driverSQLite, DataSource: "bar"},
 			nil,
 		},
 	}

extension/storage/dbstorage/extension.go

@@ -64,7 +64,7 @@ func (ds *databaseStorage) GetClient(ctx context.Context, kind component.Kind, e
 		fullName = fmt.Sprintf("%s_%s_%s_%s", kindString(kind), ent.Type(), ent.Name(), name)
 	}
 	fullName = strings.ReplaceAll(fullName, " ", "")
-	return newClient(ctx, ds.driverName, ds.db, fullName)
+	return newClient(ctx, ds.logger, ds.db, ds.driverName, fullName)
 }
 
 func kindString(k component.Kind) string {

extension/storage/dbstorage/extension_test.go

@@ -120,7 +120,7 @@ func testExtensionIntegrity(t *testing.T, se storage.Extension) {
 func newSqliteTestExtension(t *testing.T) storage.Extension {
 	f := NewFactory()
 	cfg := f.CreateDefaultConfig().(*Config)
-	cfg.DriverName = driverSqlite
+	cfg.DriverName = driverSQLite
 	cfg.DataSource = fmt.Sprintf("file:%s/foo.db?_busy_timeout=10000&_journal=WAL&_sync=NORMAL", t.TempDir())
 
 	extension, err := f.Create(context.Background(), extensiontest.NewNopSettings(), cfg)
@@ -162,7 +162,7 @@ func newPostgresTestExtension(t *testing.T) storage.Extension {
 	})
 	f := NewFactory()
 	cfg := f.CreateDefaultConfig().(*Config)
-	cfg.DriverName = driverPostgresql
+	cfg.DriverName = driverPostgreSQL
 	cfg.DataSource = fmt.Sprintf("host=%s port=%s user=%s password=%s dbname=%s sslmode=disable", "127.0.0.1", port.Port(), "root", "passwd", "db")
 
 	extension, err := f.Create(context.Background(), extensiontest.NewNopSettings(), cfg)