From e45adbc867bf9a6a8455294c52896ffc4c5ce1b5 Mon Sep 17 00:00:00 2001 From: daria-lewandowska Date: Thu, 8 Sep 2016 15:24:21 +0200 Subject: [PATCH] Added better error handling when a DB driver is wrong defined --- dbi/connect.go | 25 +++++---- dbi/dbi.go | 18 +++---- dbi/dbi_test.go | 77 +++++++++++----------------- dbi/mock/corrMockSetfile.json | 68 +++++++++++++++++++++++++ dbi/mock/incorrMockSetfile.json | 85 +++++++++++++++++++++++++++++++ dbi/mock/mockData.go | 90 ++------------------------------- dbi/ns.go | 4 +- 7 files changed, 209 insertions(+), 158 deletions(-) create mode 100644 dbi/mock/corrMockSetfile.json create mode 100644 dbi/mock/incorrMockSetfile.json diff --git a/dbi/connect.go b/dbi/connect.go index 4ab7961..615ab8b 100644 --- a/dbi/connect.go +++ b/dbi/connect.go @@ -19,7 +19,6 @@ package dbi import ( "errors" "fmt" - "os" _ "github.com/go-sql-driver/mysql" _ "github.com/lib/pq" @@ -41,7 +40,6 @@ func getDefaultPort(driver string) string { // openDB opens a database and verifies connection by calling ping to it func openDB(db *dtype.Database) error { - var dsn string // if port is not defined, set defaults @@ -61,7 +59,6 @@ func openDB(db *dtype.Database) error { default: return fmt.Errorf("SQL Driver %s is not supported", db.Driver) } - err := db.Executor.Open(db.Driver, dsn) if err != nil { return err @@ -90,11 +87,8 @@ func openDBs(dbs map[string]*dtype.Database) error { once := false for i := range dbs { err := openDB(dbs[i]) - if err != nil { - closeDB(dbs[i]) - fmt.Fprintf(os.Stderr, "Cannot open database `%+v`, err=%+v\n", dbs[i].DBName, err) - continue + return err } once = true } @@ -108,20 +102,25 @@ func openDBs(dbs map[string]*dtype.Database) error { // closeDB closes a database func closeDB(db *dtype.Database) error { - err := db.Executor.Close() - if err != nil { - return err + if db.Active { + err := db.Executor.Close() + if err != nil { + return err + } + db.Active = false } - db.Active = false return nil } // closeDBs closes databases (exported due to use in main.go) -func closeDBs(dbs map[string]*dtype.Database) { +func closeDBs(dbs map[string]*dtype.Database) []error { + //errors := []error{} + var errors []error for i := range dbs { err := closeDB(dbs[i]) if err != nil { - fmt.Fprintf(os.Stdout, "Cannot close connection to database %+v, err=%+v\n", dbs[i].DBName, err) + errors = append(errors, err) } } + return errors } diff --git a/dbi/dbi.go b/dbi/dbi.go index 112a5e0..4e928f3 100644 --- a/dbi/dbi.go +++ b/dbi/dbi.go @@ -17,7 +17,6 @@ limitations under the License. package dbi import ( - "errors" "fmt" "os" "strings" @@ -62,15 +61,12 @@ func (dbiPlg *DbiPlugin) CollectMetrics(mts []plugin.MetricType) ([]plugin.Metri // Cannot obtained sql settings return nil, err } - err = openDBs(dbiPlg.databases) if err != nil { return nil, err } - dbiPlg.initialized = true } // end of initialization - // execute dbs queries and get output data, err = dbiPlg.executeQueries() if err != nil { @@ -153,7 +149,6 @@ func (dbiPlg *DbiPlugin) getMetrics() (map[string]interface{}, error) { metrics := map[string]interface{}{} err := openDBs(dbiPlg.databases) - defer closeDBs(dbiPlg.databases) if err != nil { return nil, err @@ -165,6 +160,14 @@ func (dbiPlg *DbiPlugin) getMetrics() (map[string]interface{}, error) { return nil, err } + errors := closeDBs(dbiPlg.databases) + if errors != nil { + var dbs []string + for r := range errors { + dbs = append(dbs, errors[r].Error()) + } + return metrics, fmt.Errorf("Cannot close database(s):\n %s", dbs) + } return metrics, nil } @@ -175,7 +178,6 @@ func (dbiPlg *DbiPlugin) executeQueries() (map[string]interface{}, error) { //execute queries for each defined databases for dbName, db := range dbiPlg.databases { - if !db.Active { //skip if db is not active (none established connection) fmt.Fprintf(os.Stderr, "Cannot execute queries for database %s, is inactive (connection was not established properly)\n", dbName) @@ -187,7 +189,6 @@ func (dbiPlg *DbiPlugin) executeQueries() (map[string]interface{}, error) { statement := dbiPlg.queries[queryName].Statement out, err := db.Executor.Query(queryName, statement) - if err != nil { // log failing query and take the next one fmt.Fprintf(os.Stderr, "Cannot execute query %s for database %s", queryName, dbName) @@ -196,7 +197,6 @@ func (dbiPlg *DbiPlugin) executeQueries() (map[string]interface{}, error) { for resName, res := range dbiPlg.queries[queryName].Results { instanceOk := false - // to avoid inconsistency of columns names caused by capital letters (especially for postgresql driver) instanceFrom := strings.ToLower(res.InstanceFrom) valueFrom := strings.ToLower(res.ValueFrom) @@ -227,7 +227,7 @@ func (dbiPlg *DbiPlugin) executeQueries() (map[string]interface{}, error) { } // end of range databases if len(data) == 0 { - return nil, errors.New("No data obtained from defined queries") + return nil, fmt.Errorf("No data obtained from defined queries") } return data, nil diff --git a/dbi/dbi_test.go b/dbi/dbi_test.go index f86d5d0..5fa21a3 100644 --- a/dbi/dbi_test.go +++ b/dbi/dbi_test.go @@ -105,9 +105,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("when path to setfile is incorrect", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - // mockdata.FileName has not existed yet - deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: "./noFile.json"}) So(func() { dbiPlugin.GetMetricTypes(cfg) }, ShouldNotPanic) results, err := dbiPlugin.GetMetricTypes(cfg) So(err, ShouldNotBeNil) @@ -127,12 +125,21 @@ func TestGetMetricTypes(t *testing.T) { So(results, ShouldBeNil) }) + Convey("when there is wrong driver typed", func() { + cfg := plugin.NewPluginConfigType() + dbiPlugin := New() + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileIncorr}) + + So(func() { dbiPlugin.GetMetricTypes(cfg) }, ShouldNotPanic) + results, err := dbiPlugin.GetMetricTypes(cfg) + So(err, ShouldNotBeNil) + So(results, ShouldBeNil) + }) + Convey("when cannot open db", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} //mockExecution outputs @@ -154,9 +161,9 @@ func TestGetMetricTypes(t *testing.T) { Convey("when cannot open, neither close db", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + //createMockFile(correct) + //defer deleteMockFile() + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} //mockExecution outputs @@ -178,9 +185,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("when cannot ping the open db", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} //mockExecution outputs @@ -202,9 +207,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("when cannot switch to selected db", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} //mockExecution outputs @@ -226,9 +229,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("when execution of query returns error", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} //mockExecution outputs @@ -250,9 +251,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("when query returns empty output", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} @@ -276,9 +275,7 @@ func TestGetMetricTypes(t *testing.T) { Convey("successfully obtain metrics name", func() { cfg := plugin.NewPluginConfigType() dbiPlugin := New() - createMockFile() - defer deleteMockFile() - cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + cfg.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mc := &mcMock{stmts: make(map[string]*sql.Stmt)} @@ -319,14 +316,13 @@ func TestCollectMetrics(t *testing.T) { // set metrics config config := cdata.NewNode() - config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) for i, _ := range mts { mts[i].Config_ = config } Convey("incorrect path to setfile", func() { - // mockdata.FileName has not existed (remove it just in case) - deleteMockFile() + config.AddItem("setfile", ctypes.ConfigValueStr{Value: "./noFile.json"}) dbiPlugin := New() So(func() { dbiPlugin.CollectMetrics(mts) }, ShouldNotPanic) results, err := dbiPlugin.CollectMetrics(mts) @@ -338,6 +334,7 @@ func TestCollectMetrics(t *testing.T) { dbiPlugin := New() os.Create(mockdata.FileName) defer os.Remove(mockdata.FileName) + config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) So(func() { dbiPlugin.CollectMetrics(mts) }, ShouldNotPanic) results, err := dbiPlugin.CollectMetrics(mts) So(err, ShouldNotBeNil) @@ -350,14 +347,11 @@ func TestCollectMetrics(t *testing.T) { mts := mockdata.Mts config := cdata.NewNode() - config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) for i, _ := range mts { mts[i].Config_ = config } - createMockFile() - defer deleteMockFile() - Convey("when cannot connect to databases", func() { dbiPlugin := New() mc := &mcMock{stmts: make(map[string]*sql.Stmt)} @@ -402,8 +396,6 @@ func TestCollectMetrics(t *testing.T) { Convey("when metric's value is a timestamp - special usecase", t, func() { // to cover func fixDataType for time.Time case - createMockFile() - defer deleteMockFile() dbiPlugin := New() mc := &mcMock{stmts: make(map[string]*sql.Stmt)} @@ -420,7 +412,7 @@ func TestCollectMetrics(t *testing.T) { mts := mockdata.Mts config := cdata.NewNode() - config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) for i, _ := range mts { mts[i].Config_ = config } @@ -432,8 +424,8 @@ func TestCollectMetrics(t *testing.T) { }) Convey("collect metrics successfully", t, func() { - createMockFile() - defer deleteMockFile() + //createMockFile(correct) + //defer deleteMockFile() dbiPlugin := New() mc := &mcMock{stmts: make(map[string]*sql.Stmt)} @@ -450,7 +442,7 @@ func TestCollectMetrics(t *testing.T) { mts := mockdata.Mts config := cdata.NewNode() - config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.FileName}) + config.AddItem("setfile", ctypes.ConfigValueStr{Value: mockdata.SetfileCorr}) mts[0].Config_ = config So(func() { dbiPlugin.CollectMetrics(mts) }, ShouldNotPanic) @@ -461,14 +453,3 @@ func TestCollectMetrics(t *testing.T) { }) } - -func createMockFile() { - deleteMockFile() - - f, _ := os.Create(mockdata.FileName) - f.Write(mockdata.FileCont) -} - -func deleteMockFile() { - os.Remove(mockdata.FileName) -} diff --git a/dbi/mock/corrMockSetfile.json b/dbi/mock/corrMockSetfile.json new file mode 100644 index 0000000..bd21d15 --- /dev/null +++ b/dbi/mock/corrMockSetfile.json @@ -0,0 +1,68 @@ +{ + "queries": [ + { + "name": "q1", + "statement": "statementA", + "results": [ + { "name": "", + "instance_from": "category", + "value_from": "value" + } + ] + }, + { + "name": "q2", + "statement": "statementB", + "results": [ + { + "name": "rName1", + "instance_from": "category", + "instance_prefix": "category prefix", + "value_from": "value" + }, + { + "name": "rName2", + "instance_from": "category", + "value_from": "value" + } + ] + } + ], + "databases": [ + { + "name": "dbName1", + "driver": "mysql", + "driver_option": { + "host": "localhost", + "port": "3306", + "username": "tester", + "password": "passwd", + "dbname": "mydb" + }, + "dbqueries": [ + { + "query": "q1" + } + ] + }, + { + "name": "dbName2", + "driver": "postgres", + "driver_option": { + "host": "localhost", + "username": "tester", + "password": "passwd", + "dbname": "mydb" + }, + "selectdb": "slctdb", + "dbqueries": [ + { + "query": "q1" + }, + { + "query": "q2" + } + ] + } + ] + } \ No newline at end of file diff --git a/dbi/mock/incorrMockSetfile.json b/dbi/mock/incorrMockSetfile.json new file mode 100644 index 0000000..c920124 --- /dev/null +++ b/dbi/mock/incorrMockSetfile.json @@ -0,0 +1,85 @@ +{ + "queries": [ + { + "name": "q1", + "statement": "statementA", + "results": [ + { "name": "", + "instance_from": "category", + "value_from": "value" + } + ] + }, + { + "name": "q2", + "statement": "statementB", + "results": [ + { + "name": "rName1", + "instance_from": "category", + "instance_prefix": "category prefix", + "value_from": "value" + }, + { + "name": "rName2", + "instance_from": "category", + "value_from": "value" + } + ] + } + ], + "databases": [ + { + "name": "dbName1", + "driver": "mysql", + "driver_option": { + "host": "localhost", + "port": "3306", + "username": "tester", + "password": "passwd", + "dbname": "mydb" + }, + "dbqueries": [ + { + "query": "q1" + } + ] + }, + { + "name": "dbName2", + "driver": "postgres", + "driver_option": { + "host": "localhost", + "username": "tester", + "password": "passwd", + "dbname": "mydb" + }, + "selectdb": "slctdb", + "dbqueries": [ + { + "query": "q1" + }, + { + "query": "q2" + } + ] + }, + + { + "name": "db3", + "driver": "unknown", + "driver_option": { + "host": "localhost", + "username": "tester", + "password": "passwd", + "dbname": "mydb" + }, + + "dbqueries": [ + { + "query": "q1" + } + ] + } + ] + } \ No newline at end of file diff --git a/dbi/mock/mockData.go b/dbi/mock/mockData.go index 672c061..afdefbd 100644 --- a/dbi/mock/mockData.go +++ b/dbi/mock/mockData.go @@ -62,92 +62,8 @@ var ( } // FileName is a path of mock setfile - FileName = "./temp_setfile.json" + FileName = "temp_setfile.json" - // FileCont is a mocked content of setfile - FileCont = []byte(`{ - "queries": [ - { - "name": "q1", - "statement": "statementA", - "results": [ - { "name": "", - "instance_from": "category", - "value_from": "value" - } - ] - }, - { - "name": "q2", - "statement": "statementB", - "results": [ - { - "name": "rName1", - "instance_from": "category", - "instance_prefix": "category prefix", - "value_from": "value" - }, - { - "name": "rName2", - "instance_from": "category", - "value_from": "value" - } - ] - } - ], - "databases": [ - { - "name": "dbName1", - "driver": "mysql", - "driver_option": { - "host": "localhost", - "port": "3306", - "username": "tester", - "password": "passwd", - "dbname": "mydb" - }, - "dbqueries": [ - { - "query": "q1" - } - ] - }, - { - "name": "dbName2", - "driver": "postgres", - "driver_option": { - "host": "localhost", - "username": "tester", - "password": "passwd", - "dbname": "mydb" - }, - "selectdb": "slctdb", - "dbqueries": [ - { - "query": "q1" - }, - { - "query": "q2" - } - ] - }, - - { - "name": "db3", - "driver": "unknown", - "driver_option": { - "host": "localhost", - "username": "tester", - "password": "passwd", - "dbname": "mydb" - }, - - "dbqueries": [ - { - "query": "q1" - } - ] - } - ] - }`) + SetfileCorr = "mock/corrMockSetfile.json" + SetfileIncorr = "mock/incorrMockSetfile.json" ) diff --git a/dbi/ns.go b/dbi/ns.go index e57704c..7c3c523 100644 --- a/dbi/ns.go +++ b/dbi/ns.go @@ -16,7 +16,9 @@ limitations under the License. package dbi -import "strings" +import ( + "strings" +) // nsPrefix is prefix of metrics namespace var nsPrefix = []string{"intel", "dbi"}