-
Notifications
You must be signed in to change notification settings - Fork 82
[RFC] Rest logging improvements #978
base: master
Are you sure you want to change the base?
Changes from all commits
a4d5742
d432b5a
e4b5945
39b5ad3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,20 @@ import ( | |
| // Apache Common Log Format (CLF) | ||
| func LogRequest(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| entry := log.WithField("reqid", gdctx.GetReqID(r.Context()).String()) | ||
| f := log.Fields{ | ||
| "subsys": "rest", | ||
| "reqid": gdctx.GetReqID(r.Context()).String(), | ||
| } | ||
| if origin := r.Header.Get("X-Gluster-Origin-Args"); origin != "" { | ||
| f["origin"] = origin | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TRIVIAL: IMO origin seems a bit ambiguous, a better variable name can be chosen, something like orgnArgs.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. |
||
| } | ||
| entry := log.WithFields(f) | ||
| entry.WithFields(log.Fields{ | ||
| "method": r.Method, | ||
| "url": r.URL, | ||
| "client": r.UserAgent(), | ||
| }).Info("HTTP Request") | ||
| delete(entry.Data, logging.SourceField) | ||
| handlers.LoggingHandler(entry.Writer(), next).ServeHTTP(w, r) | ||
| handlers.CombinedLoggingHandler(entry.Writer(), next).ServeHTTP(w, r) | ||
| }) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import ( | |
| "time" | ||
|
|
||
| "github.com/gluster/glusterd2/pkg/api" | ||
| "github.com/gluster/glusterd2/version" | ||
|
|
||
| "github.com/dgrijalva/jwt-go" | ||
| ) | ||
|
|
@@ -29,11 +30,18 @@ type Client struct { | |
| password string | ||
| cacert string | ||
| insecure bool | ||
|
|
||
| // Add to the identifier to further specify the client | ||
| // using the api. | ||
| agent string | ||
| originArgs []string | ||
| } | ||
|
|
||
| // New creates new instance of Glusterd REST Client | ||
| func New(baseURL string, username string, password string, cacert string, insecure bool) *Client { | ||
| return &Client{baseURL, username, password, cacert, insecure} | ||
| return &Client{baseURL, username, password, cacert, insecure, | ||
| fmt.Sprintf("GlusterD2-rest-client/%v", version.GlusterdVersion), | ||
| []string{}} | ||
| } | ||
|
|
||
| func parseHTTPError(jsonData []byte) string { | ||
|
|
@@ -105,6 +113,7 @@ func (c *Client) do(method string, url string, data interface{}, expectStatusCod | |
| if err != nil { | ||
| return err | ||
| } | ||
| c.setAgent(req) | ||
| req.Header.Set("Accept", "application/json") | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Close = true | ||
|
|
@@ -114,6 +123,8 @@ func (c *Client) do(method string, url string, data interface{}, expectStatusCod | |
| req.Header.Set("Authorization", "bearer "+getAuthToken(c.username, c.password)) | ||
| } | ||
|
|
||
| c.sendOriginArgs(req) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: this can be named to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a I'd suggest that it be renamed to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps we keep SetOriginArgs and rename this to addOriginArgsToHeaders ? |
||
|
|
||
| tr := &http.Transport{ | ||
| DisableCompression: true, | ||
| DisableKeepAlives: true, | ||
|
|
@@ -163,3 +174,34 @@ func (c *Client) do(method string, url string, data interface{}, expectStatusCod | |
| func (c *Client) Ping() error { | ||
| return c.get("/ping", nil, http.StatusOK, nil) | ||
| } | ||
|
|
||
| func (c *Client) setAgent(req *http.Request) { | ||
| req.Header.Set("User-Agent", | ||
| fmt.Sprintf("%v (Go-http-client/1.1)", c.agent)) | ||
| } | ||
|
|
||
| // ExtendAgent adds the given string to the client's user agent | ||
| // by prefixing it to the existing agent information. | ||
| // This allows client programs to identify themselves more than | ||
| // just something using the rest client api. | ||
| func (c *Client) ExtendAgent(a string) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please rename "a" to something meaningful. |
||
| // new additions to the agent identifier go to the | ||
| // beginning of the string. It is meant to read like: | ||
| // foo (based on) bar (based on) baz, etc... | ||
| c.agent = fmt.Sprintf("%v %v", a, c.agent) | ||
| } | ||
|
|
||
| func (c *Client) sendOriginArgs(req *http.Request) { | ||
| if len(c.originArgs) != 0 { | ||
| req.Header.Set("X-Gluster-Origin-Args", | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice. is this logged? I don't see this detail in output @prashanthpai posted #978 (comment)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be, but only if the client sends it. I'm not exactly sure exactly how @prashanthpai got his examples, but when I incorporate the other feedback for these changes I'll include examples of the logging as well.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad. I think I did
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice, thanks for the update |
||
| fmt.Sprintf("1:%#v", strings.Join(c.originArgs, " "))) | ||
| } | ||
| } | ||
|
|
||
| // SetOriginArgs provides a way for tools using this library to | ||
| // inform the server what arguments were provided to generate | ||
| // the api call(s). The contents of the array are meant only for | ||
| // human interpretation but will generally be command line args. | ||
| func (c *Client) SetOriginArgs(a []string) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, please rename "a" |
||
| c.originArgs = a | ||
| } | ||
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.
nit: We can have cli version instead of glusterd version. For now it is same, but it can change in future. @prashanthpai your thoughts?
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.
IMO, this should be okay. glustercli and glusterd are almost always built together. We can quickly identify if older/outdated clients connect to more recent glusterd2 by looking at the version if they follow same versioning. A separate version for CLI will makes more sense if it's a separate repo/project.
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.
makes sense. we can update later if that happens.