Delegate RunLogsService.TailLogs to DataProxyService to remove duplicate implementation
- Dominant language
- Go
- Stars
- 7.5k
- Forks
- 886
- Avg merge
- 1d 12h
- Merged PRs (30d)
- 120
Description
## What
`RunLogsService.TailLogs` (`runs/service/run_logs_service.go:44`) and `dataproxy.Service.TailLogs` (`dataproxy/service/dataproxy_service.go:503`) are essentially doing the same thing:
1. Resolve the `LogContext` for the given action attempt.
2. Hand it to a `LogStreamer` which streams pod logs back to the client.
The canonical home for this is now the DataProxy service. The `RunLogsService.TailLogs` RPC is only kept for backward compatibility with existing clients.
## Why
Today both services:
- Depend on a `LogStreamer` implementation (`runs/service/k8s_log_streamer.go` and `dataproxy/logs/k8s_log_streamer.go` — two copies).
- Implement their own concurrency limit / context resolution code path.
- Must be kept in sync whenever we evolve log streaming behavior (filtering, pagination, new backends, auth, etc.).
This is duplicate surface area with no good reason now that DataProxy owns logs.
## Proposed change
Refactor `RunLogsService.TailLogs` into a thin shim that forwards to `DataProxyServiceClient.TailLogs`:
- Inject a `dataproxyconnect.DataProxyServiceClient` into `RunLogsService` instead of (or in addition to) the `LogStreamer`.
- In `TailLogs`:
1. Validate `action_id` / acquire the concurrency semaphore as today.
2. Open a client stream to `DataProxyService.TailLogs` with the same `action_id` + `attempt`.
3. Forward each `dataproxy.TailLogsResponse` chunk to the outbound `workflow.TailLogsResponse` stream (converting the proto message — the schemas need to be checked for 1:1 field compatibility).
- Delete `runs/service/k8s_log_streamer.go` and its test once the delegation is in place; DataProxy's `logs/k8s_log_streamer.go` becomes the single implementation.
- Keep the `runs` RPC registered so existing clients keep working.
## Open questions
- Response proto shape: confirm `workflow.TailLogsResponse` and `dataproxy.TailLogsResponse` are structurally compatible, or define a small converter.
- Wiring: the run service will need to be given a DataProxy client at startup (likely the in-process one in the all-in-one binary).
- Concurrency limit: decide whether the `defaultMaxConcurrentStreams` semaphore stays on the run-service shim, moves entirely to DataProxy, or both.
## References
- `runs/service/run_logs_service.go:44-61` — current run-service TailLogs
- `dataproxy/service/dataproxy_service.go:503-520` — DataProxy TailLogs (target delegate)
- `runs/service/k8s_log_streamer.go` / `dataproxy/logs/k8s_log_streamer.go` — duplicated streamer impls
Contributor guide
Assessment
This issue has not been assessed yet.