Skip to content
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

New access logger #1408

Merged
merged 1 commit into from
Apr 22, 2017
Merged

New access logger #1408

merged 1 commit into from
Apr 22, 2017

Conversation

rjshep
Copy link
Contributor

@rjshep rjshep commented Apr 10, 2017

As requested in #1293, splitting new access logger into smaller commits to aid PR process.

This first PR adds the new access logger alongside the existing one, allowing information to be captured but at this stage not output. Additional functionality will follow in subsequent PRs.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

It looks like the new code isn't test-covered completely yet. Do you plan on adding more tests?

Nit-pick: Could you try to fit the commit message into 80 characters?

if h, ok := crw.rw.(http.Hijacker); ok {
return h.Hijack()
}
return nil, nil, fmt.Errorf("Not a Hijacker: %T", crw.rw)
Copy link
Contributor

Choose a reason for hiding this comment

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

Decapitalize error messages please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

const (
// DataTableKey is the key within the request context used to
// store the Log Data Table
DataTableKey key = iota
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this to be extended, or why does it hold a single value only?

now := time.Now().UTC()
core := make(CoreLogData)

logDataTable := &LogData{core, req.Header, nil, nil}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to use key-indexed syntax and drop the nil values. This way, the code won't break should we add additional fields in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

const (
// DataTableKey is the key within the request context used to
// store the Log Data Table
DataTableKey key = iota
Copy link
Contributor

@timoreimann timoreimann Apr 10, 2017

Choose a reason for hiding this comment

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

Is there a particular reason why we use 0/iota as the key here? Why not a more meaningful, human-readable string?

At least, this would make debugging easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to string - more descriptive as you say.


var crr *captureRequestReader
if req.Body != nil {
crr = &captureRequestReader{req.Body, 0}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, my preference goes towards a key-indexed definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

return nil
}

func silentSplitHostPort(value string) (string, string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name the return parameters host and port, respectively, for documentary reasons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

}

// CoreLogData holds the fields computed from the request/response.
type CoreLogData map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice we have to do quite a bit of type casting due to the interface{} mapee.

Couldn't we just use a strongly typed struct instead of a map?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We went with the map so as to minimise the amount of effort needed to add additional log data in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate please? How would a strongly typed struct increase the effort? What extra work would be needed that the map does not demand?

Personally, I tend to accept a bit more effort if it brings us compile-time safety. I don't see the full picture yet to understand the different trade-offs though.

if original, ok := core[OriginContentSize]; ok {
o64 := original.(int64)
if o64 != crw.Size() {
// n.b divide-by-zero is tolerated here and dealt with elsewhere as appropriate
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean by "is tolerated here"? Wouldn't the code panic if crw.Size() turned out to be zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right it would panic. I've changed the code to avoid divide by zero.

return &SaveBackend{next, backendName}
}

func copyHeaders(src http.Header) http.Header {
Copy link
Contributor

Choose a reason for hiding this comment

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

Our vulcand/oxy dependency already brings a header copying functionality. Not sure if you can use it though since it does not override headers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code changed to use the existing header copying function.

// Temporary - until new access logger is fully implemented
// create the data table and populate frontend and backend
core := make(accesslog.CoreLogData)
logDataTable := &accesslog.LogData{core, r.Header, nil, nil}
Copy link
Contributor

Choose a reason for hiding this comment

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

Keyed index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@rjshep
Copy link
Contributor Author

rjshep commented Apr 11, 2017

I've made the requested modifications and addressed the other comments.

In terms of tests, yes there are more to come with the rest of the changes which will introduce an appender for the new logger as well as formatters for JSON and the common logging format.

The change to the existing access logger to get frontend and backend name is covered by an existing test modified in this PR.


import (
"fmt"
"github.com/vulcand/oxy/utils"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please group the non-stdlib imports at the bottom, separated by a blank line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -10,6 +10,8 @@ import (
"runtime"
"testing"

"context"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please move this import right below the other stdlib ones and use the blank line to separate from the non-stdlib imports following.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

// CoreLogData holds the fields computed from the request/response.
type CoreLogData map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate please? How would a strongly typed struct increase the effort? What extra work would be needed that the map does not demand?

Personally, I tend to accept a bit more effort if it brings us compile-time safety. I don't see the full picture yet to understand the different trade-offs though.

@rjshep
Copy link
Contributor Author

rjshep commented Apr 13, 2017

The intention for the new access logger is to provide a configurable output format, where the format, ordering and naming of the fields is based on user-supplied configuration. In order to do this simply the map was chosen so that, based on configuration, we can simply lookup the necessary values to output.

I appreciate that, as this PR doesn't include the log appenders, it's not easy to see why having the map is preferred.

If we choose a strongly typed struct we would (I believe) need to use either reflection or a large switch/case statement in order to achieve the same goal. So I guess the question is whether this is preferable to using the existing map?

If based on the above you would still like to replace the map with a struct I'll look at doing that, though my preference is to retain the map :-)

@timoreimann
Copy link
Contributor

@rjshep thanks for elaborating. I'm okay with keeping the map for now. Let's revisit the decision once the follow-up PRs are coming in and enable a better understanding as you said.

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

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

This looks good for me, approving. 👍

Thanks again for taking the time to chop down the original PR into smaller pieces. I'd be happy to review any successor PRs.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

@rjshep Thanks a lot for your contribution :)
I have one question: I think it may be safer to add these new middlewares into the chain if and only if access logs are enabled in the config. WDYT?

"net"
"net/http"
)

Copy link
Member

Choose a reason for hiding this comment

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

Could you add this here ?

var (
	_ http.ResponseWriter = &captureResponseWriter{}
	_ http.Hijacker       = &captureResponseWriter{}
	_ http.Flusher        = &captureResponseWriter{}
	_ http.CloseNotifier  = &captureResponseWriter{}
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added :-)

@rjshep
Copy link
Contributor Author

rjshep commented Apr 14, 2017

@emilevauge In terms of only adding the access log middleware when access logging is enabled in config, I think that is a good idea. If it's okay with you I propose to make that change in the next PR which will replace the current access logger with the new one?

@emilevauge
Copy link
Member

@rjshep

I propose to make that change in the next PR which will replace the current access logger with the new one?

If you can make this PR in the next days, I'm not against that (we will freeze the code for v1.3 soon).

@emilevauge emilevauge added the priority/P1 need to be fixed in next release label Apr 19, 2017
@rjshep
Copy link
Contributor Author

rjshep commented Apr 19, 2017

@emilevauge Yes I can submit the next PR in a few days.

core[StartUTC] = now
core[StartLocal] = now.Local()

r2 := req.WithContext(context.WithValue(req.Context(), DataTableKey, logDataTable))
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for r2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

core[RequestHost], core[RequestPort] = silentSplitHostPort(req.Host)
}
// copy the URL without the scheme, hostname etc
u := &url.URL{
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for u

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

ForceQuery: req.URL.ForceQuery,
Fragment: req.URL.Fragment,
}
us := u.String()
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for us

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


func usernameIfPresent(u *url.URL) string {
username := "-"
if u.User != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for u

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return &SaveBackend{next, backendName}
}

func (sb *SaveBackend) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for rw and r

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this the same as it's consistent with all of the other middlewares :-)

return &SaveFrontend{next, frontendName}
}

func (sb *SaveFrontend) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you use a more explicit variable naming for rw and r

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've left this the same as it's consistent with all of the other middlewares :-)

@rjshep
Copy link
Contributor Author

rjshep commented Apr 20, 2017

@emilevauge @ldez @timoreimann Hi all, is there anything further required before this PR can be merged? I'm keen to press on with the next PR.

@timoreimann
Copy link
Contributor

As far as my review is concerned, I'm fine with the status quo.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Thanks a lot @rjshep !
LGTM (after rebased) 🎉

@timoreimann timoreimann merged commit f38d117 into traefik:master Apr 22, 2017
@ldez ldez modified the milestone: 1.3 Apr 23, 2017
@ldez ldez added the kind/enhancement a new or improved feature. label Apr 29, 2017
@ldez ldez removed the priority/P1 need to be fixed in next release label Apr 29, 2017
@ldez ldez modified the milestones: 1.4, 1.3 May 6, 2017
@ldez ldez modified the milestones: 1.3, 1.4 May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logs kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants