Add endpoint-specific fields to API request log #3528
Conversation
dnephin
requested review from
ssoroka,
BruceMacD,
jmorganca and
pdevine
as code owners
internal/server/grants.go
Outdated
| rCtx.AddLogFields = append(rCtx.AddLogFields, func(event *zerolog.Event) { | ||
| event.Int64("lastUpdateIndex", r.LastUpdateIndex) | ||
| }) |
There was a problem hiding this comment.
I add this separately from numGrants so that if the request errors and exits early we still get this context added to the log entry.
|
Looks like INFO level: https://github.com/infrahq/infra/blob/b243a3a/internal/server/logging.go#L57 |
dnephin
force-pushed
the
dnephin/log-some-query-parameters
branch
from
d7a039f
to
2a48700
Compare
|
I made a small change to the implementation. Instead of storing the slice directly on |
|
|
||
| // Response is a mutable field. It can be modified by API handlers to add | ||
| // new response metadata. | ||
| Response *ResponseMetadata |
There was a problem hiding this comment.
consider making this a non-pointer
There was a problem hiding this comment.
I believe this will need to be a pointer so that we can mutate it without needing to call c.Set() to update the value stored in the gin.Context.
A pointer receiver may work, but I was hoping that making this a pointer make it more obvious that this was a mutable value.
I'll think about it some more .
To fix lint error on main
dnephin
force-pushed
the
dnephin/log-some-query-parameters
branch
from
2a48700
to
8be8f16
Compare
|
I pushed a commit to fix the lint failure on main. I don't think we've found an alternative without adding methods to I'm going to merge it this way. It should be super easy to change at any time. I don't see us adding all that many fields to our logs anytime soon. |
Summary
This PR introduces a hook for adding fields to the API request log. It also uses the new hook in list grants to add a
lastUpdatedIndexandnumGrantsfields. I think we may also want to use this with SCIM endpoints, so that we can track which provider is using which fields. I'm sure we'll find other use cases for adding fields to the request log as we add new endpoints.I think maybe we can find a better way to include these fields, but this seemed to work for now. If anyone has a suggestion for a better interface, I'd be happy to incorporate it.