Add support for customizing log file directory (logsDir) in Multi-App… - #1653
Add support for customizing log file directory (logsDir) in Multi-App…#1653codevisionary-omer wants to merge 1 commit into
Conversation
|
bump |
1e0d44a to
ac790f6
Compare
|
Hey @codevisionary-omer Can you fix git in this branch? There're lots of commits that don't seem right. I'd expect only 2995f1c in this PR. |
ac790f6 to
82b5482
Compare
|
@acroca Sorry about that fixed |
… Run template Log files for app and daprd were always written to <appDirPath>/.dapr/logs with no way to change the location. This writes constant filesystem events into the project tree, which degrades dev servers that recursively watch it (e.g. uvicorn --reload via watchfiles), and prevents centralizing logs. This change adds an optional 'logsDir' field to the Multi-App Run template, available under 'common' and per app. Precedence: apps[i].logsDir > common.logsDir > default (<appDirPath>/.dapr/logs). Relative paths are resolved against the app directory (per-app) or the run file directory (common); '~' is expanded. The directory is created on use. Default behavior is unchanged when the field is not set. Signed-off-by: YOUR NAME <YOUR_EMAIL> Signed-off-by: Omer Spalter <omer@codevisionary.ai>
82b5482 to
e65f875
Compare
There was a problem hiding this comment.
Pull request overview
Adds configurable logsDir support to the Multi-App Run (dapr run -f) template so app/daprd log files can be written outside <appDirPath> (helping dev file watchers and enabling centralized log collection), while preserving the current default behavior when unset.
Changes:
- Introduces
logsDirfields incommonand per-app config, with precedenceapp.logsDir > common.logsDir > default (<appDirPath>/.dapr/logs). - Resolves
logsDirvalues to absolute paths (including~expansion) without requiring the directory to exist at parse time. - Adds tests and a runfile fixture covering precedence and path resolution behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/runfileconfig/testdata/test_run_config_logs_dir.yaml | Adds a runfile fixture exercising common/app override behavior for logsDir. |
| pkg/runfileconfig/run_file_config.go | Adds LogsDir fields and updates GetLogsDir() to honor configured log directories. |
| pkg/runfileconfig/run_file_config_parser.go | Resolves logsDir to absolute paths and applies common→app inheritance during validation. |
| pkg/runfileconfig/run_file_config_parser_test.go | Adds tests for logsDir precedence, resolution, and creation behavior. |
Comments suppressed due to low confidence (1)
pkg/runfileconfig/run_file_config_parser_test.go:450
- t.Cleanup removes a hard-coded absolute path ("/tmp/dapr-abs-logs"). If that directory already exists on a developer/CI machine, this test will delete unrelated files. Cleanup should only remove directories created under the test’s control.
t.Cleanup(func() {
os.RemoveAll(expectedCommonLogsDir)
os.RemoveAll(expectedApp2LogsDir)
os.RemoveAll("/tmp/dapr-abs-logs")
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Run("app's absolute logsDir is used as is", func(t *testing.T) { | ||
| assert.Equal(t, "/tmp/dapr-abs-logs", apps[2].LogsDir) | ||
| assert.Equal(t, "/tmp/dapr-abs-logs", apps[2].GetLogsDir()) | ||
| }) |
There was a problem hiding this comment.
This is legit, but the suggested fix doesn't fully solve it: the hard-coded /tmp/... path isn't absolute on Windows (filepath.IsAbs is false without a volume), so the parser joins it onto appDirPath and the LogsDir assertion fails at parse time even without calling GetLogsDir(). Since CI runs unit tests on windows-latest, this would fail there.
I'd generate the run file in the test with an absolute path from t.TempDir() instead and cleanup comes for free.
Description
When using Multi-App Run (
dapr run -f), app and daprd log files are always written to<appDirPath>/.dapr/logs, with no way to change the location. This causes two problems:--reload, which uses watchfiles) receive constant filesystem events from log writes insideappDirPath, causing churn and a performance penalty. Watcher exclusion filters don't help, because the OS-level recursive watch still delivers the events — the fix is for logs to live outside the watched tree.Setting
appLogDestination/daprdLogDestination: consoleavoids the file writes but loses persisted logs, so it is a workaround rather than a solution.This PR adds an optional
logsDirfield to the Multi-App Run template, available undercommonand per app:Behavior:
apps[i].logsDir>common.logsDir> default (<appDirPath>/.dapr/logs).appDirPath; relativecommonpaths resolve against the run file's directory (consistent with other paths in the template).~is expanded.logsDiris not set, behavior is unchanged.Changes:
pkg/runfileconfig/run_file_config.go— addedLogsDirfield onAppandCommon;GetLogsDir()honors it.pkg/runfileconfig/run_file_config_parser.go— resolveslogsDirto absolute paths (newresolvePathToAbshelper that does not require the path to exist) and applies precedence.pkg/runfileconfig/run_file_config_parser_test.go— tests for precedence, relative/absolute resolution, directory creation, and unchanged default behavior.pkg/runfileconfig/testdata/test_run_config_logs_dir.yaml— test fixture.Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #1652
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: