Feature/note feature transfer #17

lkwdwrd · 2019-04-22T18:27:41Z

Features and ideas created with the note service, transferred here.

ConnectionManager bootstrap and shutdown service connections gracefully in concurrently
DB Updates: Allow db object to work will with ConnectionManager
HTTP Helpers: Decode and encode JSON consistently
Logger: Configurable output logger, WIP
OS Env: Allow a default value version in addition to the MustGet version
Middleware: Consolidate functionality, add a TrimSlash middleware option.
Server: Remove server startup method -- too much abstraction
Test Helpers: Encode/decode helpers and a status response assertion with context (we should avoid assert functions in general, but this helper really made the note tests more readable).
Validation: Helpers for checking the encoding and formatting for E3DB style values

…e interface method

efabens · 2019-04-23T23:38:06Z

connectionmgr.go

+// ConnectionManager manages initialization and shutdown of long lived connections.
+// Each connection object must match the Initializer interface. Initialization happens
+// in parallel. The waitgroup can be used to wait util all connections are initialized.
+// Close kick off the proper shutdown of all managed connections.


s/kick/kicks

galxy25 · 2019-04-24T20:03:36Z

connectionmgr.go

@@ -0,0 +1,72 @@
+package utils


Every consumer of utils doesn't need this file, it should be put into a separate package for this module. Suggest connection_manager.go

galxy25 · 2019-04-24T20:05:07Z

connectionmgr.go

+}
+
+// NewConnectionManager initializes a new ConnectionManager object that can be used
+// to manage the life of long lived remote connections such as to a database.


Did you look at/think about using context for this purpose? https://blog.golang.org/context

galxy25 · 2019-04-24T20:07:50Z

connectionmgr.go

+
+// DoInit initializes an object matching the Initializer interface, setting its close
+// operation to run when the Connection Manager's Close method is called.
+func (cm *ConnectionManager) DoInit(initializer Initializer) {


Since this is the major functionality of connection manager, I'd think about having the initializer be a field of connection manager and pass it in when calling New for connection manager.

galxy25 · 2019-04-24T20:09:26Z

database/db.go

@@ -59,16 +61,31 @@ func (d dbLogger) AfterQuery(q *pg.QueryEvent) {

 // New returns a new DB object which wraps
 // a connection to the database specified in config
-func New(config DBConfig) DB {
+func New(config DBConfig, logger *log.Logger) DB {


prefer passing logger in as a param of config versus changing the signature of this method

galxy25 · 2019-04-24T20:09:44Z

database/db.go

 	db := pg.Connect(&pg.Options{
 		Addr:     config.Address,
 		User:     config.User,
 		Database: config.Database,
 		Password: config.Password,
 	})
-	if os.Getenv("QUERY_DEBUG") == "ON" {


Tears of joy to see this removed.

galxy25 · 2019-04-24T20:15:38Z

database/db.go

+	}
+}
+
+// Initialize starts up the database and returns the Close function used to gracefully shut it down.


this comment needs to be updated to be accurate, there is no database started, only an attempt to run migrations(which as a consumer of this code I would like to know is going to happen when I call this code), which I don't think should happen as it prevents someone from using this who wants to do something non standard/consider the database initialized in a different way.. Please keep in mind when adding code to this module that the purpose of utils is to provide flexible general purpose code, not become the abstract class that imposes behavior on consumers.

galxy25 · 2019-04-24T20:18:04Z

http.go

@@ -162,3 +162,25 @@ func HealthCheckHandler(serviceName string) http.Handler {
 		w.Write([]byte(fmt.Sprintf("%s service is up.\n", serviceName)))
 	})
 }
+
+// ReqToObject un-marshals a request object body JSON into an object
+func ReqToObject(r *http.Request, obj interface{}) error {


nit: Please follow the convention elsewhere in this code and most of go of spelling things out, I don't see any logic to why we would excludeuest but not 'ject' .

s/object/struct/

Suggest DeserializeJsonRequest as a more accurate, go'ish name.

galxy25 · 2019-04-24T20:20:41Z

http.go

+
+// WriteObjectResponse marshals an object into the response body and sets
+// JSON content type headers
+func WriteObjectResponse(obj interface{}, w http.ResponseWriter) error {


Suggest SerializeJsonResponse for this function name.

galxy25 · 2019-04-24T20:21:04Z

logger.go

@@ -72,3 +73,18 @@ func NewServiceLogger(serviceName string, level int) ServiceLogger {
 	}
 	return logger
 }
+
+// GetLogger initializes a logging object writing to the requested log file.
+func GetLogger(logFile string, serviceName string) *log.Logger {


Suggest the more go'ish name of "MakeLogger"

galxy25 · 2019-04-24T20:22:35Z

logger.go

@@ -72,3 +73,18 @@ func NewServiceLogger(serviceName string, level int) ServiceLogger {
 	}
 	return logger
 }
+
+// GetLogger initializes a logging object writing to the requested log file.
+func GetLogger(logFile string, serviceName string) *log.Logger {


this should accept the interface it cares about (io.writer), not a magic string that is impartially checked for usefulness

galxy25 · 2019-04-24T20:24:49Z

validation.go

@@ -0,0 +1,43 @@
+package utils


this should also be a separate subpackge

lkwdwrd · 2019-05-02T13:33:43Z

database/db.go

+	User          string
+	Database      string
+	Password      string
+	Logger        *log.Logger


Should this just be made a Service Logger at this point? Do we fully standardize on it?

This can be a service logger - its at the point now with the last updates Michael made to not have a logger per level and accept human readable log level that its ready for prime time/doesn't have any major usability issues I can see(take that with a grain of salt as I haven't used it in any of the code I've written)

It's my proudest go code. That aside, switching between the two is fairly painless and none of the consumer code for the logger will have to change.

lkwdwrd · 2019-05-02T13:49:04Z

server/middleware.go

+
+// JSONLoggingMiddleware wraps an HTTP handler and logs
+// the request and de-serialized JSON body.
+func JSONLoggingMiddleware(logger *log.Logger) Middleware {


Again -- wondering if fully embracing the service logger is better here?

lkwdwrd · 2019-05-02T13:50:39Z

@galxy25 @efabens - big updates based on reviews and conversations. Reset review status and now it could use another look.

The changes are a little heavy handed, but after talking with Levi, I decided to fully embrace some of the concepts.

The biggest potential breaking change is middleware. I moved the old middleware functions to live with the middleware type turned them into full fledged middleware functions rather than wrapping them.

Along with the change, I moved http utils into the server package. I actually wanted to name the package http, but since this would be used with the net/http package 99% of the time, I didn't think it was worth the naming conflict. server is the best I came up with for now, but happy if you have other thoughts. The utils are broken out into what feels like some logical files separations. I have no doubt this subpackage will grow as utils-go gets bigger. Maybe this should be the root utils package?

I refactored the service logger with some ideas based on conversations. I'm hoping the API is a little more flexible. It is now, essentially, a decorated log.Logger, so things like setting log output, prefix, etc. are supported. Along with Printf style Print and Println are style log level functions are available as well. @willmichael - thank you for you work on this! This is not necessarily a better way, just different, and I wanted to get this version up for discussion. Please comment on this take if you have the time!

Connection Manager is broken out to manage Initialization and Close separately and the API for adding managed connections changed to have better names. More (better?) doc strings.

The DB struct initialization was refactored based on the golang bufio.Scanner struct in terms of how to allow Initialization method customization. See buffio.Scanner.Split.

Naming updates, sub packages. Sorry for the mess!

galxy25 · 2019-05-02T19:15:16Z

connectionmanager/connectionmanager.go

+		cm.ManageClose(ic)
+		cm.ManageInitialization(ic)
+	}
+}


This file looks like beautiful go code :-)

wow, high praise! Thanks!

galxy25 · 2019-05-02T19:25:19Z

database/db.go

 }

 // DB wraps a client for a database.
 type DB struct {
-	Client *pg.DB
+	Client      *pg.DB
+	Logger      *log.Logger


I think this should be embedded into this struct and not a public field(I can't think of a use case other code would have for needing access to the database's logger), with the additional upside that methods on this struct can make logging statements via db.Println() versus db.Logger.Println
https://golang.org/doc/effective_go.html#embedding
https://travix.io/type-embedding-in-go-ba40dd4264df

Note though that a service logger satisfies the logger interface so can be passed as the logger value here, and that the underlying Postgres client only accepts a logger interface so won't be able to take advantage of any additional methods we have(though our database package can)

As far as I know log.Logger is still a concrete type, not an interface :(

golang/go#13182

willmichael · 2019-05-02T19:25:32Z

connectionmanager/connectionmanager.go

@@ -0,0 +1,118 @@
+package connectionmanager


connectionmanager feels like an eyesore to me, suggest package connection if it doesn't conflict with anything else?

I've fought a lot with this name. Happy for something better!

Connection isn't quite accurate since this manages the initialization/shutdown of any number of items as long as they support the initializer/closer interfaces. Indeed, connection is probably even wrong, though it nods to the intent of this managing things like database/elastic search connections. It provides a method of bringing them up and shutting them down as a group, rather than individually. I don't have a good name, so I stuck with connection manager. The file used to be called connectionmgr, but Levi set me straight on short names.

connectiongroup? Naming is hard, I'll come back to it if I can think of something...

willmichael · 2019-05-02T19:28:52Z

connectionmanager/connectionmanager.go

+// Closers are queued up internally running only when the ConnectionManager's Close method
+// is called. The ConnectionManager runs each Close method in a separate go routine and blocks
+// blocks until all are complete.
+type ConnectionManager struct {


I believe a more idiomatic naming scheme would be to remove Connection prefix from the names within this package to avoid the stutter

Manager feels too generic to me as a name - but again, I can't seem to name this functionality. Help!

Manager as a whole is generic but it's within the context of connectionmanager: the consumer of the package will be typing connectionmanager.ConnectionManager repeatedly. But it could be connectionmanager.Manager or even connectiongroup.Manager

galxy25 · 2019-05-02T19:31:45Z

logging/servicelogger.go

+	if sl.logLevel >= levelMap[level] {
+		sl.Output(2, sl.prefixString(level)+fmt.Sprintln(v...))
+	}
+}


Now that we have this I think its safe/time to delete https://github.com/tozny/utils-go/blob/master/logger.go#L10

logger.go is removed as part of this PR (it was moved here and refactored).

Also much better comments than me 👍

willmichael · 2019-05-02T19:32:31Z

connectionmanager/connectionmanager.go

+	Close()
+}
+
+// InitializerCloser is the interface the both initializes and gracefully closes a


that both

willmichael · 2019-05-02T19:33:38Z

connectionmanager/connectionmanager.go

+
+// ManageConnection accepts any number of items matching the InitializerCloser
+// interface and manages both an item's initialization and close.
+func (cm *ConnectionManager) ManageConnection(initializerClosers ...InitializerCloser) {


ManageConnection seems to restart a connection, close -> initialize -> return. Would a more accurate name be Restart?

It doesn't...

Though the ordering is a bit confusing, this queues up the close method of the managed item first (which will not run until the connection manager itself has the Close method called. Once queued, it then adds the items initialization function, which immediately starts a go routine that initializes the item. The order ensure the close function is managed before initialization begins.

The fact that you are asking means this is just a bit too confusing. I'll add additional comments to clarify.

Ahh I understand, maybe EnqueueConnection/EnqueueClosers/EnqueueInitializers more accurately describes what they're doing?

galxy25 · 2019-05-02T19:34:54Z

server/handlers.go

+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(fmt.Sprintf("%s service is up.\n", serviceName)))
+	})
+}


If you want these methods to live here(which seems reasonable), I think we should delete the duplicate methods here https://github.com/tozny/utils-go/blob/master/http.go

galxy25 · 2019-05-02T19:37:05Z

server/server.go

-		final = handler(final)
+// ExtractBearerToken attempts to extract an Oauth bearer token
+// from the provided request, returning extracted token and error (if any)
+func ExtractBearerToken(r *http.Request) (string, error) {


Please cleanup the now deprecated version of this in https://github.com/tozny/utils-go/blob/master/http.go

galxy25 · 2019-05-02T19:37:23Z

server/server.go

-// Middleware is a function that decorates an http.Handler
-type Middleware func(handler http.Handler) http.Handler
+var (
+	// SupportedAuthTypes is a whitelist of supported authentication types. Deafult: Bearer


please delete now deprecated values for the below from https://github.com/tozny/utils-go/blob/master/http.go

galxy25 · 2019-05-02T19:37:48Z

server/middleware.go

-// JSONLoggingMiddleware converts the utils JsonLogger into Middleware
-func JSONLoggingMiddleware(logger *log.Logger) Middleware {
+const (
+	// ToznyClientIDHeader is the headers key containing a client ID


please delete now deprecated values for the below from https://github.com/tozny/utils-go/blob/master/http.go

galxy25 · 2019-05-02T19:38:06Z

server/encoding.go

+
+// HandleError is a generic error handler for responding with the given status and error
+// using the provided ResponseWriter.
+func HandleError(w http.ResponseWriter, statusCode int, response interface{}) {


please delete now deprecated values for the below from https://github.com/tozny/utils-go/blob/master/http.go

http.go is removed as part of this pr and broken up / added to the server subpackage

willmichael · 2019-05-02T19:45:03Z

database/db.go

 func New(config DBConfig) DB {
 	db := pg.Connect(&pg.Options{
 		Addr:     config.Address,
 		User:     config.User,
 		Database: config.Database,
 		Password: config.Password,
 	})
-	if os.Getenv("QUERY_DEBUG") == "ON" {
-		db.AddQueryHook(dbLogger{logger: log.New(os.Stdout, fmt.Sprintf("%s: %s: ", config.Address, config.Database), log.LstdFlags)})
+	if config.EnableLogging {


Trying to think about how the Service Logger can be integrated into this, so we don't have to specifically enable at the DB level... But everything I can think of is slightly convoluted. Unless we want to restrict the DB logging to a specific level like debug

Luke Woodward added 7 commits April 17, 2019 11:43

Initial move of helper from storage service

a909806

Refactored to remove the need to return close function from Initializ…

2869de1

…e interface method

switch bakc to looped close

019a26c

Waitgroup and parallelize shutdown

afb0430

Waitgroup and parallelize shutdown

bada0cb

Move over a couple test utilities

016f5fd

Remove unneeded routing helper

0a656a6

lkwdwrd requested review from eddier, fernviridian, efabens, galxy25, willmichael and jennatuckerdeveloper April 22, 2019 18:27

Fix comment verbiage

1456d77

efabens approved these changes Apr 23, 2019

View reviewed changes

galxy25 reviewed Apr 24, 2019

View reviewed changes

validation.go Outdated

@@ -0,0 +1,43 @@

package utils

Copy link

Contributor

galxy25 Apr 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also be a separate subpackge

galxy25 approved these changes Apr 24, 2019

View reviewed changes

Luke Woodward added 3 commits May 1, 2019 16:00

Merge branch 'master' into feature/note-feature-transfer

8f8bf6e

Refactor based on utils discussion

ebbfc17

Additional updates based on code review

deeaff4

Luke Woodward added 4 commits May 1, 2019 22:32

Use new only for connection manager

03da30f

Adjust and refine the Middleware api

34486d8

Fix middleware syntax issues

6d0b18f

Actually display assertion context

3363262

lkwdwrd requested review from efabens and galxy25 May 2, 2019 13:32

lkwdwrd commented May 2, 2019

View reviewed changes

Luke Woodward added 2 commits May 2, 2019 06:35

channel is a go keyword

97d919d

Better naming

a360d37

lkwdwrd commented May 2, 2019

View reviewed changes

galxy25 reviewed May 2, 2019

View reviewed changes

willmichael reviewed May 2, 2019

View reviewed changes

galxy25 reviewed May 2, 2019

View reviewed changes

willmichael reviewed May 2, 2019

View reviewed changes

galxy25 reviewed May 2, 2019

View reviewed changes

galxy25 approved these changes May 2, 2019

View reviewed changes

willmichael reviewed May 2, 2019

View reviewed changes

Updated comment based on review

853cda4

willmichael approved these changes May 2, 2019

View reviewed changes

Commit to service logger in utils-go

ee7e9c4

lkwdwrd merged commit ff4e135 into master May 2, 2019

lkwdwrd deleted the feature/note-feature-transfer branch May 2, 2019 21:29

Feature/note feature transfer #17

Feature/note feature transfer #17

Conversation

lkwdwrd commented Apr 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lkwdwrd commented May 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willmichael May 2, 2019 • edited Loading

Choose a reason for hiding this comment

lkwdwrd commented May 2, 2019 •

edited

Loading

willmichael May 2, 2019 •

edited

Loading