-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Bookkeeping #35
Add Bookkeeping #35
Conversation
This looks good to me. |
src/scytale/primaryHandler.go
Outdated
@@ -132,21 +145,6 @@ func NewPrimaryHandler(logger log.Logger, v *viper.Viper, registry xmetrics.Regi | |||
logginghttp.SetLogger( | |||
logger, | |||
logginghttp.RequestInfo, | |||
|
|||
// custom logger func that extracts the intended destination of requests | |||
func(kv []interface{}, request *http.Request) []interface{} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are you removing this block of code?
@njharter can this be merged? |
We don't want to be doing bookkeeping this way, in any of our servers:
|
There are (3) basic steps to a better solution. The first (2) need only be done once, for all servers. (1) Use
(2) Create a bookkeeping decorator, preferably in one place such as the
(3) Use both of the above decorators for any handlers in application code:
|
src/scytale/primaryHandler.go
Outdated
func doFanout(ctx context.Context, fanout *http.Request, message *wrp.Message) (context.Context, error) { | ||
populateMessage(ctx, message) | ||
|
||
bookkeepingLog(ctx, message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want this here. This is in application-layer code. We want the bookkeeping done as part of an Alice-style decorator. This decorator can go into the logginghttp
package as a standard thing applications can add to their handler mappings.
src/scytale/primaryHandler.go
Outdated
return ctx, nil | ||
} | ||
|
||
func bookkeepingLog(ctx context.Context, message *wrp.Message) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should not exist in application code. What you want is decoration:
package logginghttp
import (
"github.com/Comcast/webpa-common/logging"
"net/http"
)
func Bookkeeper(next http.Handler) http.Handler {
return http.HandlerFunc(func(response http.ResponseWriter, request *http.Request) {
// rely on another decorator, e.g. xcontext.Populate, to supply the fields
logging.GetLogger(request.Context()).Log(logging.MessageKey(), "Bookkeeping")
next.ServeHTTP(response, request)
})
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Client code, such as in scytale
, will then supply a contextual logger, again via decoration:
// imports omitted
c := xcontext.Populate(0,
logginghttp.SetLogger(loggerFromViper, logginghttp.Header("X-Example", "example")),
)
// use 'c' to decorate handlers:
alice.New(c, /* other constructors).Then(myHandler)
|
||
var message wrp.Message | ||
|
||
switch request.Header.Get("Content-Type") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoding the message is not a very efficient way of accomplishing this. Also, we don't want to require bookkeeping to have a dependence on wrp
. We should find a way to do this that doesn't require decoding, perhaps by inserting the wrp
message from the fanout into the context
.
Closes #34