Implement DMR log streaming via /logs endpoint#807
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors log handling by centralizing merging and following logic into a new pkg/logs package and introduces a /logs API endpoint for remote log access. Feedback was provided regarding the timestamp parsing format string, which should use placeholders for variable-length fractional seconds to ensure correct chronological merging.
9cc2414 to
28e7fe4
Compare
…no-engines options
28e7fe4 to
cd3f9b8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the log fetching and merging logic into a dedicated pkg/logs package and introduces a new /logs HTTP endpoint to support log streaming in Docker Desktop environments where local files might be inaccessible. The CLI now supports falling back to this API. Feedback was provided regarding the pollMode setting in the HTTP handler; since the server runs in a Linux VM even on Windows hosts, the current OS check is insufficient to enable polling for host-mounted volumes, which is necessary for reliable log tailing.
| // Follow: tail from recorded offsets using a flushing writer. | ||
| // pollMode is needed on Windows where filesystem event | ||
| // notifications are unreliable. | ||
| pollMode := runtime.GOOS == "windows" |
There was a problem hiding this comment.
The pollMode is only enabled when runtime.GOOS is "windows". However, the model runner server executes within the Docker Desktop VM, where runtime.GOOS will be "linux", regardless of the host OS. When Docker Desktop runs on a Windows host, log files are on a host-mounted volume where filesystem notifications are unreliable, making polling necessary for tail to function correctly. Without polling, the log streaming feature (--follow) will likely not work for users on Windows.
To ensure reliability across all host platforms, polling should be enabled unconditionally for the HTTP handler.
| pollMode := runtime.GOOS == "windows" | |
| pollMode := true |
There was a problem hiding this comment.
DMR runs on the host no in the DD VM
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The timestamp layout in pkg/logs/merge.go looks incorrect: Go time layouts must use the reference digits "000000000" for the fractional seconds, but timeFmt is defined as "2006-01-02T15:04:05.999999999Z", which will cause time.Parse to fail on otherwise valid log timestamps; this should likely be reverted to use zeros.
- In pkg/logs/http_handler.go, errors from MergeLogs are written into the response body after a 200 status and partial content may already be sent; if you need clients to distinguish failure, consider buffering the merge output or performing a lightweight pre-check so that you can send an appropriate non-200 status before any body bytes are written.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timestamp layout in pkg/logs/merge.go looks incorrect: Go time layouts must use the reference digits "000000000" for the fractional seconds, but timeFmt is defined as "2006-01-02T15:04:05.999999999Z", which will cause time.Parse to fail on otherwise valid log timestamps; this should likely be reverted to use zeros.
- In pkg/logs/http_handler.go, errors from MergeLogs are written into the response body after a 200 status and partial content may already be sent; if you need clients to distinguish failure, consider buffering the merge output or performing a lightweight pre-check so that you can send an appropriate non-200 status before any body bytes are written.
## Individual Comments
### Comment 1
<location path="cmd/cli/commands/logs.go" line_range="84" />
<code_context>
+ if localErr != nil {
+ // Local files unavailable (e.g. running inside a container).
+ // Fall back to the DMR /logs API.
+ apiErr := desktopClient.Logs(
+ cmd.Context(), follow, noEngines, cmd.OutOrStdout(),
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** fmt.Errorf is used with two %w verbs, which will panic at runtime; only a single %w is allowed.
You’ll need to wrap only one of these errors and format the other normally. For example:
```go
return fmt.Errorf(
"local logs unavailable (%v); API fallback failed: %w",
localErr, apiErr,
)
```
or:
```go
return fmt.Errorf(
"local logs unavailable: %w; API fallback failed: %v",
localErr, apiErr,
)
```
</issue_to_address>
### Comment 2
<location path="pkg/logs/follow.go" line_range="56-58" />
<code_context>
+
+ // Engine log is optional; a missing file is tolerated.
+ var et *tail.Tail
+ if engineLogPath != "" {
+ et, err = tail.TailFile(engineLogPath, makeCfg(offsets.EngineOffset))
+ if err != nil {
+ et = nil
+ } else {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Errors opening the engine log for tailing are silently ignored, which can hide real problems and drop engine logs.
If `tail.TailFile` fails (e.g., permissions, I/O issues), the code just sets `et = nil` and continues, so the caller gets a nil error while engine logs are never tailed. Since `MergeLogs` may have already read from the engine log, this silent drop is misleading. Please either return/log the error here, or treat only "file not found" as non-fatal, e.g.:
```go
if err != nil {
if os.IsNotExist(err) {
et = nil
} else {
return fmt.Errorf("tail engine log: %w", err)
}
}
```
Suggested implementation:
```golang
import (
"os"
```
```golang
// Engine log is optional; a missing file is tolerated.
var et *tail.Tail
if engineLogPath != "" {
et, err = tail.TailFile(engineLogPath, makeCfg(offsets.EngineOffset))
if err != nil {
if os.IsNotExist(err) {
// Missing engine log is non-fatal; just skip tailing it.
et = nil
} else {
return fmt.Errorf("tail engine log: %w", err)
}
} else {
defer et.Cleanup()
defer func(et *tail.Tail) {
_ = et.Stop()
}(et)
}
}
```
1. Ensure `"os"` is not already imported; if it is, merge the import lines instead of duplicating.
2. Keep the import block formatted according to `gofmt` (the Go tooling will usually reorder imports automatically).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
doringeman
left a comment
There was a problem hiding this comment.
I don't see the llama.cpp logs if I run it on the host on macOS on MODEL_RUNNER_HOST=http://localhost:8080 docker model run smollm2 hi.
|
This ^ is not a bug introduced by this PR though. |
ericcurtin
left a comment
There was a problem hiding this comment.
LGTM, pending passing tests
@doringeman yeah this PR does not solve this issue. The goal of this PR is to solve the issue where DMR runs on a host via Docker Desktop and you run |
Implement DMR log streaming via
/logsendpointAdds a
GET /logsHTTP endpoint and refactors thedocker model logsCLI command to share log merging and tailing logic through a newpkg/logspackage.What's new:
GET /logsendpoint — streams service and engine logs merged in chronological order. Enabled whenMODEL_RUNNER_LOG_DIRis set.?follow=true(real-time tail),?no-engines=true(exclude engine logs)docker model logsCLI command now supports both local file access and remote API fallbackpkg/logspackage withMergeLogsandFollowfunctions, covered by unit testsUsage