fix(httpserver): replace stdlib log with structured logr in memory handler#1773
Conversation
…ndler Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
mesutoezdil
requested review from
iplay88keys,
jmhbh and
supreme-gg-gg
as code owners
github-actions
Bot
added
bug
There was a problem hiding this comment.
Pull request overview
Updates the MemoryHandler logging to use structured request-scoped logging instead of the stdlib log, aligning the memory endpoints with the logging approach used across the httpserver package.
Changes:
- Replace
log.Printfcalls with structuredInfo/Errorlogging. - Retrieve a logger from the request context in memory handler endpoints.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| log.Printf("Successfully added %d memory items", len(memories)) | ||
| log.Info("added memory batch", "count", len(memories)) |
There was a problem hiding this comment.
Consider using a consistent log key naming convention for structured fields (most handlers use camelCase keys like count, userID, agentName). Using count is fine, but if you add more fields later, stick to the established key style to keep log queries consistent.
|
|
||
| // List handles GET /api/memories and returns all memories for an agent+user, ranked by access frequency | ||
| func (h *MemoryHandler) List(w ErrorResponseWriter, r *http.Request) { | ||
| log := logr.FromContextOrDiscard(r.Context()) |
There was a problem hiding this comment.
Using logr.FromContextOrDiscard(r.Context()) will discard logs if the handler is invoked without the logging middleware attaching a logger to the context. To keep behavior consistent with other handlers (and with the previous stdlib logger always emitting), prefer ctrllog.FromContext(r.Context()) and add an operation/name as needed.
|
|
||
| // Delete handles DELETE /api/memories | ||
| func (h *MemoryHandler) Delete(w ErrorResponseWriter, r *http.Request) { | ||
| log := logr.FromContextOrDiscard(r.Context()) |
There was a problem hiding this comment.
Using logr.FromContextOrDiscard(r.Context()) here can drop logs when no logger is present in context. Align with other handlers by using ctrllog.FromContext(r.Context()) (and optionally setting .WithName("memory-handler") / .WithValues("operation", "delete")) so logs still emit outside the middleware path (including unit tests).
|
|
||
| // AddSession handles POST /api/memories/sessions | ||
| func (h *MemoryHandler) AddSession(w ErrorResponseWriter, r *http.Request) { | ||
| log := logr.FromContextOrDiscard(r.Context()) |
There was a problem hiding this comment.
Using logr.FromContextOrDiscard(r.Context()) will silently drop logs when the request context doesn't have a logger (e.g., handler methods called directly in unit tests or if middleware is bypassed). Other handlers consistently use ctrllog.FromContext(r.Context()) (controller-runtime) which falls back to the global logger and composes correctly with ctrllog.IntoContext in the logging middleware. Consider switching to ctrllog.FromContext(...).WithName("memory-handler").WithValues("operation", "add-session") (and similarly for other operations) to avoid losing logs and to match handler logging patterns.
|
|
||
| // AddSessionBatch handles POST /api/memories/sessions/batch | ||
| func (h *MemoryHandler) AddSessionBatch(w ErrorResponseWriter, r *http.Request) { | ||
| log := logr.FromContextOrDiscard(r.Context()) |
There was a problem hiding this comment.
Using logr.FromContextOrDiscard(r.Context()) here can discard logs when the context isn't populated by the logging middleware. To match the rest of the handlers and preserve fallback behavior, prefer ctrllog.FromContext(r.Context()) (optionally adding .WithName("memory-handler") / .WithValues("operation", "add-session-batch")).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Eitan Yarmush <eitan.yarmush@solo.io>
Replaced logr.FromContextOrDiscard with ctrllog.FromContext in all four handler functions. FromContextOrDiscard silently drops logs when no logger is in the context; ctrllog.FromContext falls back to the global logger so logs still emit if middleware is bypassed. Also aligned log field keys with the rest of the handlers: user_id -> userID, agent -> agentName. Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
The memory handler was the only handler in the package still using the stdlib
logpackage. Every other handler usesgo-logr/logr. The logging middleware already injects a logr logger into the request context viactrllog.IntoContext, so switching tologr.FromContextOrDiscard(r.Context())is a straight drop-in.This makes memory handler logs consistent with the rest: same format, same log level control, same structured key/value fields.