Feat/store processed stats in pgsql database #12

c1-ra · 2022-03-08T23:41:38Z

Description

Enable to store computed stats in a pgsql database.

Changes

Add a StatsService with connection to the database
Add a Create() method to this service computed stats in database.

Notes

/

c1-ra · 2022-03-09T11:50:44Z

I have prepared the insertCodestats statement to add to Create(), using #8 current work.
It works when I test it, but waiting for #8 to be merged to change some imports before adding the commit.
Feel free to review meanwhile though, since the action of inserting codestats will be almost like the previous ones (except for the statement).

GregoryAlbouy · 2022-03-09T12:06:42Z

postgresql/error.go

+var (
+	// ErrEnvironmentVariable is returned when the worker fails to find
+	// the environment variables necessary to connect to the database.
+	ErrEnvironmentVariable = errors.New("could not find all necessary environment variables")
+	// ErrDatabaseConnection is returned when the worker fails to connect to
+	// the database.
+	ErrDatabaseConnection = errors.New("database connection error")
+	// ErrDatabasePing is returned when the worker fails to ping the
+	// database.
+	ErrDatabasePing = errors.New("database ping error")
+	// ErrPreparingStmt is returned when the worker fails to prepare
+	// a prepared statement.
+	ErrPreparingStmt = errors.New("error executing prepared statement")
+	// ErrExecutingPreparedStmt is returned when the worker fails to execute
+	// a query with a prepared statement.
+	ErrExecutingPreparedStmt = errors.New("error executing prepared statement")
+	// ErrScanningRows is returned when the worker fails to scan the rows
+	// returned by a query.
+	ErrScanningRows = errors.New("error trying to scan result rows")
+	// ErrExecutingRollback is returned when the worker fails to rollback
+	// in a transaction.
+	ErrExecutingRollback = errors.New("error trying to rollback a transaction in database")
+)


[suggestion] I'd skip error handling on this repo and always return raw errors: they'll be logged for us (not users) so let's not obfuscate them

Suggested change

var (

// ErrEnvironmentVariable is returned when the worker fails to find

// the environment variables necessary to connect to the database.

ErrEnvironmentVariable = errors.New("could not find all necessary environment variables")

// ErrDatabaseConnection is returned when the worker fails to connect to

// the database.

ErrDatabaseConnection = errors.New("database connection error")

// ErrDatabasePing is returned when the worker fails to ping the

// database.

ErrDatabasePing = errors.New("database ping error")

// ErrPreparingStmt is returned when the worker fails to prepare

// a prepared statement.

ErrPreparingStmt = errors.New("error executing prepared statement")

// ErrExecutingPreparedStmt is returned when the worker fails to execute

// a query with a prepared statement.

ErrExecutingPreparedStmt = errors.New("error executing prepared statement")

// ErrScanningRows is returned when the worker fails to scan the rows

// returned by a query.

ErrScanningRows = errors.New("error trying to scan result rows")

// ErrExecutingRollback is returned when the worker fails to rollback

// in a transaction.

ErrExecutingRollback = errors.New("error trying to rollback a transaction in database")

)

GregoryAlbouy · 2022-03-09T12:14:03Z

postgresql/connection.go

+func GetConfigFromEnvVariables() (Config, error) {
+	var config Config
+
+	err := godotenv.Load(".env")
+	// no error returned here because .env is not deployed
+	if err != nil {
+		fmt.Println("no .env file found")
+	}
+
+	config.Host = os.Getenv("PSQL_HOST")
+	if config.Host == "" {
+		return config, ErrEnvironmentVariable
+	}
+
+	config.User = os.Getenv("PSQL_USER")
+	if config.User == "" {
+		return config, ErrEnvironmentVariable
+	}
+
+	config.Password = os.Getenv("PSQL_PASSWORD")
+	if config.Password == "" {
+		return config, ErrEnvironmentVariable
+	}
+
+	config.DBName = os.Getenv("PSQL_NAME")
+	if config.DBName == "" {
+		return config, ErrEnvironmentVariable
+	}
+
+	config.IdleConn = 10
+	config.MaxConn = 25
+
+	return config, nil
+}


[suggestion] I'd rather do this in the main function:

cfg, err := envConfig() // very early in the function statsService, err := postgresql.NewStatsService(cfg) // connection is ok, compute stuff and store to db

What do you mean by main function? In a main package? There is none at the moment.
I thought we could only connect when the computing of the stats has ended 🤔

Here the "main" function is worker.Digest.
Why compute all the data if in the end we cannot connect to the database? That's why I suggest to make all the necessary checks upstream

On the opposite, I was thinking why bother connecting to the database if we fail to compute the data? 😅

I'd say because the computing part is supposed to be the heavier and longer task (that's the reason we have a worker after all) 😛
It's debatable for the connection part I agree, however I'm convinced the params checking at least should be done before anything. Connecting to the db right after getting the config also allows to gather related logics by using the config closer to where it's retrieved.
But maybe we'll want to keep that connection as short as possible (i.e. closed during processing), in which case we'd better do it at the end.
🤷‍♂️

Ok, you've convinced me, let's do it at the beginning of worker.Digestsince worker should indeed be heavier!

postgresql/stats.go

postgresql/connection.go

postgresql/stats.go

benchttp/service.go

benchttp/stats.go

postgresql/connection.go

postgresql/stats.go

Fix `Create()` signature and change name to `Insert`'

+ rebase on main

+ slight refactor of Insert() method to use less parameters

moreirathomas

Suggestion regarding Config location.

Do we want to wait for auth flow for UserID before merging ?

moreirathomas · 2022-03-10T17:30:22Z

benchttp/stats.go

+
+// Config contains the information needed to connect to the database
+// and set max idle connections and open connections number.
+type Config struct {


Config of postgresql db does not belong to benchttp package I think.

Simply move it to postegresql and we're fine :)

moreirathomas · 2022-03-10T17:33:09Z

main.go

 // Digest is a Cloud Function triggered by a Firestore create document
 // event to extract, compute and store statistics of a Benchttp run.
+// It also stores the computed data in a SQL database.


Suggested change

// Digest is a Cloud Function triggered by a Firestore create document

// event to extract, compute and store statistics of a Benchttp run.

// It also stores the computed data in a SQL database.

// Digest is a Cloud Function triggered by a Firestore create document

// event to extract, compute and store statistics of a Benchttp run

// database a SQL database.

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch from 57d8238 to 0521bf3 Compare March 8, 2022 23:43

c1-ra linked an issue Mar 8, 2022 that may be closed by this pull request

Store processed stats in sql database #5

Open

2 tasks

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch 4 times, most recently from 84d2784 to 6758db7 Compare March 9, 2022 11:29

c1-ra marked this pull request as ready for review March 9, 2022 11:48

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch from c84cf7a to e25a58f Compare March 9, 2022 11:57

GregoryAlbouy reviewed Mar 9, 2022

View reviewed changes

postgresql/stats.go Show resolved Hide resolved

moreirathomas reviewed Mar 9, 2022

View reviewed changes

benchttp/service.go Outdated Show resolved Hide resolved

benchttp/stats.go Show resolved Hide resolved

postgresql/connection.go Outdated Show resolved Hide resolved

postgresql/connection.go Outdated Show resolved Hide resolved

postgresql/stats.go Outdated Show resolved Hide resolved

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch from cbb51a7 to 5f43e4d Compare March 9, 2022 14:42

C added 14 commits March 9, 2022 15:50

Add service with connection to database

c96859d

Add benchttp.stats.go with necessary structs to interact with database

ac89531

Add postgresql.stats.go with Create() method

5218040

Add insertIntoTimestats statement in Create()

e5492e6

Add ErrExecutingRollback error

815f7cf

Change statement names

f949585

Add new lines

fc3bef7

Remove custom errors

009132f

Fix if err multiple conditions to not override errors

fb03af5

Remove .env file

af7e19c

Change service name to InsertionService

ac31012

Fix `Create()` signature and change name to `Insert`'

Change StatsService to InsertionService

f919ba3

Create a new InsertionService in main.go

b09d274

Add codestats insertion

bcde53e

+ rebase on main

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch 2 times, most recently from 25a0da7 to 635a7c4 Compare March 9, 2022 19:09

Add buildStats() method, call NewInsertService() in main.go

035836a

+ slight refactor of Insert() method to use less parameters

c1-ra force-pushed the feat/store-processed-stats-in-pgsql-database branch from 635a7c4 to 035836a Compare March 9, 2022 19:12

Add documentation comments

c865578

moreirathomas reviewed Mar 10, 2022

View reviewed changes

feat: get id from report in firestore

0ff18d6

Feat/store processed stats in pgsql database #12

Are you sure you want to change the base?

Feat/store processed stats in pgsql database #12

Uh oh!

Conversation

c1-ra commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Notes

Uh oh!

c1-ra commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GregoryAlbouy Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

GregoryAlbouy Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

c1-ra Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

GregoryAlbouy Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

c1-ra Mar 9, 2022

Choose a reason for hiding this comment

Uh oh!

GregoryAlbouy Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1-ra Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moreirathomas left a comment

Choose a reason for hiding this comment

Uh oh!

moreirathomas Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

moreirathomas Mar 10, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

c1-ra commented Mar 8, 2022 •

edited

Loading

c1-ra commented Mar 9, 2022 •

edited

Loading

GregoryAlbouy Mar 9, 2022 •

edited

Loading

c1-ra Mar 9, 2022 •

edited

Loading