From e966cc037b936bd3e828fc43db85d5a8e6dbb998 Mon Sep 17 00:00:00 2001 From: Fergal Mc Carthy Date: Mon, 22 Jul 2024 12:44:26 -0400 Subject: [PATCH] Finish switching from log to slog for telemetry client Also converts log.Fatal() calls to slog.Error() followed by return in many cases, especially those within the internal packages of the telemetry client library. Added missing checks for errors when decompressing data for item rows. Defined named return values for some of the datastore methods to simplify error handling and return statements. Minor other code cleanups. Closes: #15 --- cmd/clientds/main.go | 34 ++++-- cmd/generator/main.go | 54 ++++++++-- pkg/config/config.go | 2 - pkg/lib/bundles.go | 8 +- pkg/lib/datastore.go | 210 +++++++++++++++++++++++++++----------- pkg/lib/limits.go | 9 +- pkg/lib/processor_test.go | 23 +++-- pkg/lib/reports.go | 8 +- 8 files changed, 259 insertions(+), 89 deletions(-) diff --git a/cmd/clientds/main.go b/cmd/clientds/main.go index 5989e22..dc02c98 100644 --- a/cmd/clientds/main.go +++ b/cmd/clientds/main.go @@ -3,7 +3,7 @@ package main import ( "flag" "fmt" - "log" + "log/slog" "github.com/SUSE/telemetry/pkg/client" "github.com/SUSE/telemetry/pkg/config" @@ -34,7 +34,12 @@ func main() { cfg, err := config.NewConfig(opts.config) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to load specified config", + slog.String("config", opts.config), + slog.String("Error", err.Error()), + ) + panic(err) } fmt.Printf("Config: %+v\n", cfg) @@ -48,7 +53,12 @@ func main() { tc, err := client.NewTelemetryClient(cfg) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to instantiate TelemetryClient", + slog.String("config", opts.config), + slog.String("Error", err.Error()), + ) + panic(err) } processor := tc.Processor() @@ -56,7 +66,11 @@ func main() { if opts.items { itemRows, err := processor.GetItemRows() if err != nil { - log.Fatal(err.Error()) + slog.Error( + "Failed to retrieve items from client datastore", + slog.String("error", err.Error()), + ) + panic(err) } itemCount := len(itemRows) @@ -71,7 +85,11 @@ func main() { if opts.bundles { bundleRows, err := processor.GetBundleRows() if err != nil { - log.Fatal(err.Error()) + slog.Error( + "Failed to retrieve bundles from client datastore", + slog.String("error", err.Error()), + ) + panic(err) } bundleCount := len(bundleRows) @@ -86,7 +104,11 @@ func main() { if opts.reports { reportRows, err := processor.GetReportRows() if err != nil { - log.Fatal(err.Error()) + slog.Error( + "Failed to retrieve reports from client datastore", + slog.String("error", err.Error()), + ) + panic(err) } reportCount := len(reportRows) diff --git a/cmd/generator/main.go b/cmd/generator/main.go index c90bf09..8c39ead 100644 --- a/cmd/generator/main.go +++ b/cmd/generator/main.go @@ -3,7 +3,7 @@ package main import ( "flag" "fmt" - "log" + "log/slog" "os" "github.com/SUSE/telemetry/pkg/client" @@ -40,7 +40,12 @@ func main() { cfg, err := config.NewConfig(opts.config) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to load config", + slog.String("config", opts.config), + slog.String("error", err.Error()), + ) + panic(err) } fmt.Printf("Config: %+v\n", cfg) @@ -54,37 +59,64 @@ func main() { tc, err := client.NewTelemetryClient(cfg) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to instantiate TelemetryClient", + slog.String("config", opts.config), + slog.String("error", err.Error()), + ) + panic(err) } err = tc.Register() if err != nil { - log.Fatal(err) + slog.Error( + "Failed to register TelemetryClient", + slog.String("error", err.Error()), + ) + panic(err) } for _, jsonFile := range opts.jsonFiles { jsonContent, err := os.ReadFile(jsonFile) if err != nil { - log.Fatal(fmt.Errorf("error reading contents of telemetry JSON file: %s", err)) + slog.Error( + "Error reading telemetry data", + slog.String("jsonFile", jsonFile), + slog.String("error", err.Error()), + ) + panic(err) } err = tc.Generate(opts.telemetry, jsonContent, opts.tags) if err != nil { - log.Fatal(fmt.Errorf("error generating a telemetry data item from JSON file '%s': %s", jsonFile, err)) + slog.Error( + "Error generating telemetry data item", + slog.String("jsonFile", jsonFile), + slog.String("error", err.Error()), + ) + panic(err) } } // create one or more bundles from available data items if !opts.nobundles { if err := tc.CreateBundles(opts.tags); err != nil { - log.Fatal(fmt.Errorf("error telemetry bundles: %s", err)) + slog.Error( + "Error creating telemetry bundles", + slog.String("error", err.Error()), + ) + panic(err) } } // create one or more reports from available bundles if !opts.noreports { if err := tc.CreateReports(opts.tags); err != nil { - log.Fatal(fmt.Errorf("error creating telemetry reports: %s", err)) + slog.Error( + "Error creating telemetry reports", + slog.String("error", err.Error()), + ) + panic(err) } } @@ -92,7 +124,11 @@ func main() { // submit available reports. if !opts.nosubmit { if err := tc.Submit(); err != nil { - log.Fatal(fmt.Errorf("error submitting telemetry: %s", err)) + slog.Error( + "Error submitting telemetry", + slog.String("error", err.Error()), + ) + panic(err) } } } diff --git a/pkg/config/config.go b/pkg/config/config.go index 244ac05..72dd5ec 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -3,7 +3,6 @@ package config import ( "encoding/json" "fmt" - "log" "log/slog" "os" "slices" @@ -94,7 +93,6 @@ func NewConfig(cfgFile string) (*Config, error) { return cfg, fmt.Errorf("failed to read contents of config file '%s': %s", cfgFile, err) } - log.Printf("Contents: %q", contents) slog.Info("Contents", slog.String("contents", string(contents))) err = yaml.Unmarshal(contents, &cfg) if err != nil { diff --git a/pkg/lib/bundles.go b/pkg/lib/bundles.go index d4f6646..e830823 100644 --- a/pkg/lib/bundles.go +++ b/pkg/lib/bundles.go @@ -2,7 +2,6 @@ package telemetrylib import ( "database/sql" - "log" "log/slog" "strings" @@ -136,7 +135,12 @@ func (b *TelemetryBundleRow) Insert(db *sql.DB, itemIDs []int64) (bundleId strin for _, itemID := range itemIDs { _, err := db.Exec("UPDATE items SET bundleId = ? WHERE id = ?", b.Id, itemID) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to update bundleId in item", + slog.Int64("itemId", itemID), + slog.String("error", err.Error()), + ) + return "", err } } diff --git a/pkg/lib/datastore.go b/pkg/lib/datastore.go index c22be4e..9ec1e7e 100644 --- a/pkg/lib/datastore.go +++ b/pkg/lib/datastore.go @@ -3,7 +3,6 @@ package telemetrylib import ( "database/sql" "fmt" - "log" "log/slog" "os" "path/filepath" @@ -36,11 +35,18 @@ func NewDatabaseStore(dbConfig config.DBConfig) (ds *DatabaseStore, err error) { // create file if it doesn't already exist, without truncating it if it does. file, err := os.OpenFile(dbPath, os.O_RDONLY|os.O_CREATE, 0600) if err != nil { - log.Fatal(err.Error()) + slog.Error( + "Failed to open(O_CREATE) SQLite file", + slog.String("dbPath", dbPath), + slog.String("error", err.Error()), + ) return nil, err } file.Close() - slog.Info("created SQLite file", slog.String("filePath", dbPath)) + slog.Debug( + "created SQLite file", + slog.String("dbPath", dbPath), + ) } // exsure foreign_keys and journal_mode options are specified @@ -177,7 +183,12 @@ func (d *DatabaseStore) GetItems(bundleIds ...any) (itemRowIds []int64, itemRows // NOTE: Query() extra args must be of type any hence queryIds is type []any rows, err := d.Conn.Query(query, queryBundleIds...) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to retrieve items with specified bundleIds", + slog.Any("bundleIds", bundleIds), + slog.String("error", err.Error()), + ) + return } defer rows.Close() @@ -194,21 +205,37 @@ func (d *DatabaseStore) GetItems(bundleIds ...any) (itemRowIds []int64, itemRows &itemRow.ItemChecksum, &itemRow.Compression, &itemRow.BundleId); err != nil { - log.Fatal(err) + slog.Error( + "Failed to scan item row", + slog.String("error", err.Error()), + ) + return nil, nil, err } // ItemData can be stored as compressed data itemRow.ItemData, err = utils.DecompressWhenNeeded(itemRow.ItemData, itemRow.Compression) + if err != nil { + slog.Error( + "Failed to decompress item data", + slog.String("itemId", itemRow.ItemId), + slog.String("error", err.Error()), + ) + return nil, nil, err + } itemRows = append(itemRows, &itemRow) itemRowIds = append(itemRowIds, itemRow.Id) } - if err := rows.Err(); err != nil { - log.Fatal(err) + if err = rows.Err(); err != nil { + slog.Error( + "Failed to process retrieved item rows", + slog.String("error", err.Error()), + ) + return } - return itemRowIds, itemRows, err + return } func (d *DatabaseStore) GetBundles(reportIds ...any) (bundleRowIds []int64, bundleRows []*TelemetryBundleRow, err error) { @@ -223,7 +250,12 @@ func (d *DatabaseStore) GetBundles(reportIds ...any) (bundleRowIds []int64, bund // NOTE: Query() extra args must be of type any hence queryIds is type []any rows, err := d.Conn.Query(query, queryBundleIds...) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to retrieve bundles with specified reportIds", + slog.Any("reportIds", reportIds), + slog.String("error", err.Error()), + ) + return } defer rows.Close() @@ -239,18 +271,25 @@ func (d *DatabaseStore) GetBundles(reportIds ...any) (bundleRowIds []int64, bund &bundleRow.BundleAnnotations, &bundleRow.BundleChecksum, &bundleRow.ReportId); err != nil { - log.Fatal(err) + slog.Error( + "Failed to scan bundle row", + slog.String("error", err.Error()), + ) + return nil, nil, err } bundleRows = append(bundleRows, &bundleRow) bundleRowIds = append(bundleRowIds, bundleRow.Id) } - if err := rows.Err(); err != nil { - log.Fatal(err) + if err = rows.Err(); err != nil { + slog.Error( + "Failed to process retrieved bundle rows", + slog.String("error", err.Error()), + ) + return } - return bundleRowIds, bundleRows, err - + return } func (d *DatabaseStore) GetReports(ids ...any) (reportRowIds []int64, reportRows []*TelemetryReportRow, err error) { @@ -265,7 +304,12 @@ func (d *DatabaseStore) GetReports(ids ...any) (reportRowIds []int64, reportRows // NOTE: Query() extra args must be of type any hence queryIds is type []any rows, err := d.Conn.Query(query, queryIds...) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to retrieve reports with specified ids", + slog.Any("ids", ids), + slog.String("error", err.Error()), + ) + return } defer rows.Close() @@ -279,21 +323,28 @@ func (d *DatabaseStore) GetReports(ids ...any) (reportRowIds []int64, reportRows &reportRow.ReportClientId, &reportRow.ReportAnnotations, &reportRow.ReportChecksum); err != nil { - log.Fatal(err) + slog.Error( + "Failed to scan report row", + slog.String("error", err.Error()), + ) + return nil, nil, err } reportRows = append(reportRows, &reportRow) reportRowIds = append(reportRowIds, reportRow.Id) } - if err := rows.Err(); err != nil { - log.Fatal(err) + if err = rows.Err(); err != nil { + slog.Error( + "Failed to process retrieved report rows", + slog.String("error", err.Error()), + ) + return } - return reportRowIds, reportRows, err + return } -func (d *DatabaseStore) GetItemCount(bundleIds ...any) (int, error) { - var count int +func (d *DatabaseStore) GetItemCount(bundleIds ...any) (count int, err error) { // generate the SQL count query statement for the items table query, queryIds := genSqlCountQuery( "items", @@ -302,15 +353,19 @@ func (d *DatabaseStore) GetItemCount(bundleIds ...any) (int, error) { bundleIds, ) // NOTE: Query() extra args must be of type any hence queryIds is type []any - err := d.Conn.QueryRow(query, queryIds...).Scan(&count) + err = d.Conn.QueryRow(query, queryIds...).Scan(&count) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to count items associated with specified bundles", + slog.Any("bundleIds", bundleIds), + slog.String("error", err.Error()), + ) + return } - return count, err + return } -func (d *DatabaseStore) GetBundleCount(reportIds ...any) (int, error) { - var count int +func (d *DatabaseStore) GetBundleCount(reportIds ...any) (count int, err error) { // generate the SQL count query statement for the bundles table query, queryIds := genSqlCountQuery( "bundles", @@ -319,15 +374,19 @@ func (d *DatabaseStore) GetBundleCount(reportIds ...any) (int, error) { reportIds, ) // NOTE: Query() extra args must be of type any hence queryIds is type []any - err := d.Conn.QueryRow(query, queryIds...).Scan(&count) + err = d.Conn.QueryRow(query, queryIds...).Scan(&count) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to count bundles associated with specified reports", + slog.Any("reportIds", reportIds), + slog.String("error", err.Error()), + ) + return } - return count, err + return } -func (d *DatabaseStore) GetReportCount(ids ...any) (int, error) { - var count int +func (d *DatabaseStore) GetReportCount(ids ...any) (count int, err error) { // generate the SQL count query statement for the reports table query, queryIds := genSqlCountQuery( "reports", @@ -336,11 +395,16 @@ func (d *DatabaseStore) GetReportCount(ids ...any) (int, error) { ids, ) // NOTE: Query() extra args must be of type any hence queryIds is type []any - err := d.Conn.QueryRow(query, queryIds...).Scan(&count) + err = d.Conn.QueryRow(query, queryIds...).Scan(&count) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to count reports with specified ids", + slog.Any("ids", ids), + slog.String("error", err.Error()), + ) + return } - return count, err + return } func (d *DatabaseStore) GetDataItemRowsInABundle(bundleId string) (itemRows []*TelemetryDataItemRow, err error) { @@ -348,38 +412,58 @@ func (d *DatabaseStore) GetDataItemRowsInABundle(bundleId string) (itemRows []*T rows, err := d.Conn.Query(`SELECT items.id, items.itemId, items.itemType, items.itemTimestamp, items.itemAnnotations, items.itemData, items.itemChecksum, items.compression, items.bundleId FROM items JOIN bundles ON items.bundleId = bundles.id WHERE bundles.bundleId = ?`, bundleId) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to retrieve items with specified bundleId", + slog.String("bundleId", bundleId), + slog.String("error", err.Error()), + ) + return } defer rows.Close() for rows.Next() { - var dataitemRow TelemetryDataItemRow + var itemRow TelemetryDataItemRow if err := rows.Scan( - &dataitemRow.Id, - &dataitemRow.ItemId, - &dataitemRow.ItemType, - &dataitemRow.ItemTimestamp, - &dataitemRow.ItemAnnotations, - &dataitemRow.ItemData, - &dataitemRow.ItemChecksum, - &dataitemRow.Compression, - &dataitemRow.BundleId); err != nil { - log.Fatal(err) + &itemRow.Id, + &itemRow.ItemId, + &itemRow.ItemType, + &itemRow.ItemTimestamp, + &itemRow.ItemAnnotations, + &itemRow.ItemData, + &itemRow.ItemChecksum, + &itemRow.Compression, + &itemRow.BundleId); err != nil { + slog.Error( + "Failed to scan item row", + slog.String("error", err.Error()), + ) + return nil, err } // ItemData can be stored as compressed data - dataitemRow.ItemData, err = utils.DecompressWhenNeeded(dataitemRow.ItemData, dataitemRow.Compression) + itemRow.ItemData, err = utils.DecompressWhenNeeded(itemRow.ItemData, itemRow.Compression) + if err != nil { + slog.Error( + "Failed to decompress item data", + slog.String("itemId", itemRow.ItemId), + slog.String("error", err.Error()), + ) + return nil, err + } - itemRows = append(itemRows, &dataitemRow) + itemRows = append(itemRows, &itemRow) } - if err := rows.Err(); err != nil { - log.Fatal(err) + if err = rows.Err(); err != nil { + slog.Error( + "Failed to process retrieved item rows", + slog.String("error", err.Error()), + ) + return } - return itemRows, err - + return } func (d *DatabaseStore) GetBundleRowsInAReport(reportId string) (bundleRows []*TelemetryBundleRow, err error) { @@ -387,7 +471,12 @@ func (d *DatabaseStore) GetBundleRowsInAReport(reportId string) (bundleRows []*T rows, err := d.Conn.Query(`SELECT bundles.id, bundles.bundleId, bundles.bundleTimestamp, bundles.bundleClientId, bundles.bundleCustomerId, bundles.bundleAnnotations, bundles.bundleChecksum, bundles.reportId FROM bundles JOIN reports ON bundles.reportId = reports.id WHERE reports.reportId = ?`, reportId) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to retrieve bundles with specified reportId", + slog.String("reportId", reportId), + slog.String("error", err.Error()), + ) + return } defer rows.Close() @@ -403,17 +492,24 @@ func (d *DatabaseStore) GetBundleRowsInAReport(reportId string) (bundleRows []*T &bundleRow.BundleAnnotations, &bundleRow.BundleChecksum, &bundleRow.ReportId); err != nil { - log.Fatal(err) + slog.Error( + "Failed to scan bundle row", + slog.String("error", err.Error()), + ) + return nil, err } bundleRows = append(bundleRows, &bundleRow) } - if err := rows.Err(); err != nil { - log.Fatal(err) + if err = rows.Err(); err != nil { + slog.Error( + "Failed to process retrieved bundle rows", + slog.String("error", err.Error()), + ) + return } return bundleRows, err - } // only for testing diff --git a/pkg/lib/limits.go b/pkg/lib/limits.go index 6958958..50b3d89 100644 --- a/pkg/lib/limits.go +++ b/pkg/lib/limits.go @@ -2,7 +2,6 @@ package telemetrylib import ( "errors" - "log" "log/slog" ) @@ -43,8 +42,7 @@ func (t *TelemetryDataLimits) GetTelemetryDataLimits() TelemetryDataLimits { // CheckLimits checks the telemetry data limits func (t *TelemetryDataLimits) CheckLimits(data []byte) error { dataSize := uint64(len(data)) - log.Println("Checking size limits for Telemetry Data") - slog.Info( + slog.Debug( "Checking size limits for Telemetry Data", slog.Uint64("Data size", dataSize), slog.Uint64("Max", t.MaxSize), @@ -58,7 +56,10 @@ func (t *TelemetryDataLimits) CheckLimits(data []byte) error { case dataSize < t.MinSize: return errors.New("payload size is below the minimum limit") default: - slog.Info("Checks passed") + slog.Debug( + "Acceptable telemetry data size", + slog.Uint64("Data size", dataSize), + ) return nil } } diff --git a/pkg/lib/processor_test.go b/pkg/lib/processor_test.go index b117753..2f1b90b 100644 --- a/pkg/lib/processor_test.go +++ b/pkg/lib/processor_test.go @@ -2,7 +2,7 @@ package telemetrylib import ( "fmt" - "log" + "log/slog" "strings" "testing" @@ -28,12 +28,20 @@ func NewProcessorTestEnv(cfgFile string) (*telemetryProcessorTestEnv, error) { func (t *telemetryProcessorTestEnv) setup() (err error) { t.cfg, err = config.NewConfig(t.cfgPath) if err != nil { - log.Print(err.Error()) + slog.Error( + "Failed to load config", + slog.String("cfgPath", t.cfgPath), + slog.String("error", err.Error()), + ) return } processor, err := NewTelemetryProcessor(&t.cfg.DataStores) if err != nil { - log.Printf("failed to setup telemetry processor for config %q: %s", t.cfgPath, err.Error()) + slog.Error( + "Failed to setup telemetry processor", + slog.String("cfgPath", t.cfgPath), + slog.String("error", err.Error()), + ) return } t.telemetryprocessor = processor @@ -130,7 +138,6 @@ func (t *TelemetryProcessorTestSuite) TestCreateBundle() { bundleRow, berr := telemetryprocessor.GenerateBundle(1, "customer id", btags) if berr != nil { - log.Printf("Failed to create the bundle") t.Fail("Test failed to create the bundle") } @@ -208,7 +215,6 @@ func (t *TelemetryProcessorTestSuite) TestReport() { btags := types.Tags{types.Tag("key1=value1"), types.Tag("key2")} bundleRow, berr := telemetryprocessor.GenerateBundle(1, "customer id", btags) if berr != nil { - log.Printf("Failed to create the bundle") t.Fail("Test failed to create the bundle") } @@ -242,7 +248,6 @@ func (t *TelemetryProcessorTestSuite) TestReport() { btags1 := types.Tags{types.Tag("key3=value3"), types.Tag("key4")} bundleRow, berr = telemetryprocessor.GenerateBundle(1, "customer id", btags1) if berr != nil { - log.Printf("Failed to create the bundle") t.Fail("Test failed to create the bundle") } @@ -370,7 +375,11 @@ func addDataItems(totalItems int, processor TelemetryProcessor) error { formattedJSON := fmt.Sprintf(payload, utils.GenerateRandomString(3)) err := processor.AddData(telemetryType, []byte(formattedJSON), tags) if err != nil { - log.Printf("Failed to add the item %d", numItems) + slog.Error( + "Failed to add the item", + slog.Int("numItems", numItems), + slog.String("error", err.Error()), + ) return err } diff --git a/pkg/lib/reports.go b/pkg/lib/reports.go index edf6519..c168a84 100644 --- a/pkg/lib/reports.go +++ b/pkg/lib/reports.go @@ -2,7 +2,6 @@ package telemetrylib import ( "database/sql" - "log" "log/slog" "strings" @@ -120,7 +119,12 @@ func (r *TelemetryReportRow) Insert(db *sql.DB, bundleIDs []int64) (reportId str for _, bundleID := range bundleIDs { _, err := db.Exec("UPDATE bundles SET ReportId = ? WHERE id = ?", r.Id, bundleID) if err != nil { - log.Fatal(err) + slog.Error( + "Failed to update reportId in bundle", + slog.Int64("bundleId", bundleID), + slog.String("error", err.Error()), + ) + return "", err } } reportId = r.ReportId