-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Feat/log/uuid #4392
Feat/log/uuid #4392
Conversation
Every log event resulting from a command will have a uuid associated with it License: MIT Signed-off-by: Forrest Weston <forrest@protocol.ai>
License: MIT Signed-off-by: Forrest Weston <forrest@protocol.ai>
5c8e135
to
1c3e590
Compare
path/resolver.go
Outdated
@@ -135,6 +135,7 @@ func (s *Resolver) ResolvePathComponents(ctx context.Context, fpath Path) ([]nod | |||
if err != nil { | |||
return nil, err | |||
} | |||
defer log.EventBegin(ctx, "resolvePathComponents", logging.LoggableMap{"parts": parts, "cid": h}).Done() |
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.
In the case that there is an error in the execution of this method, do we want to add a loggable error to the event log (see L143)?
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, yes. We'd probably also want to log the error from L146 as well. Also, this event should probably go at the top and include any errors from SplitAbsPath
.
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.
You can save the result value of EventBegin
and add additional data to it.
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.
Overall, LGTM.
path/resolver.go
Outdated
@@ -135,6 +135,7 @@ func (s *Resolver) ResolvePathComponents(ctx context.Context, fpath Path) ([]nod | |||
if err != nil { | |||
return nil, err | |||
} | |||
defer log.EventBegin(ctx, "resolvePathComponents", logging.LoggableMap{"parts": parts, "cid": h}).Done() |
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, yes. We'd probably also want to log the error from L146 as well. Also, this event should probably go at the top and include any errors from SplitAbsPath
.
commands/http/handler.go
Outdated
@@ -137,6 +138,7 @@ func (i internalHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
} | |||
|
|||
ctx, cancel := context.WithCancel(node.Context()) | |||
ctx = logging.ContextWithLoggable(ctx, loggables.Uuid("requestId")) |
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.
I would move this down below the defer cancel.
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.
One nitpick, then LGTM
If an error occurs during an event add it to the events metadata License: MIT Signed-off-by: Forrest Weston <forrest@protocol.ai>
@victorbjelkholm whatsup with jenkins? |
Add an uuid/requestId to root context - this will add a requestId to event logs resulting from a command
Add event logs around path resolution - said logs make use of requestId