Skip to content

feat: add OutputFile option for log destination in logger#152

Merged
JoshVanL merged 7 commits intodapr:mainfrom
MyMirelHub:logfile-out
Apr 9, 2026
Merged

feat: add OutputFile option for log destination in logger#152
JoshVanL merged 7 commits intodapr:mainfrom
MyMirelHub:logfile-out

Conversation

@MyMirelHub
Copy link
Copy Markdown
Contributor

@MyMirelHub MyMirelHub commented Mar 25, 2026

Description

This PR adds support for writing logger output to a file.

Changes made:

  • Added OutputFile to logger options and initialized it in defaults.
  • Added a new --log-file CLI flag in AttachCmdFlags.
  • Updated ApplyOptionsToLoggers to open the configured file and route logger output to it.
  • Added tests for default option values, flag registration, and file-output behavior.

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: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
@MyMirelHub MyMirelHub marked this pull request as ready for review March 31, 2026 20:25
@MyMirelHub MyMirelHub requested review from a team as code owners March 31, 2026 20:25
@JoshVanL JoshVanL requested a review from Copilot April 1, 2026 12:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds support for directing logger output to a user-specified file via a new option/flag, and validates the behavior with unit tests.

Changes:

  • Introduces Options.OutputFile and registers a new --log-file CLI flag.
  • Updates ApplyOptionsToLoggers to open the configured file and route logger output to it.
  • Adds tests covering default values, flag registration, and file-output behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
logger/options.go Adds OutputFile option + --log-file flag and applies file output in ApplyOptionsToLoggers.
logger/options_test.go Extends options/flag tests and adds a test that verifies logs are written to the configured file.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

nelson-parente and others added 2 commits April 8, 2026 10:54
- Relax file permissions from 0600 to 0644 to allow reads (per Albert's review)
- Track open log file handle in package-level state protected by mutex
  to prevent FD leaks when ApplyOptionsToLoggers is called multiple times
- Close previous log file before opening a new one on re-apply
- Revert output to stdout when OutputFile is empty (handles config reload)
- Extract setLogOutput helper for clarity
- Add TestApplyOptionsToLoggersFileOutputReapply test verifying
  re-apply closes the previous file and switches to the new one

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
@MyMirelHub
Copy link
Copy Markdown
Contributor Author

Follow-up on the review feedback:

  • Addressed the FD leak/re-apply behavior by tracking the active log file handle with package-level state + mutex, closing/replacing it on re-apply, and reverting output to stdout when OutputFile is empty.
  • Updated file mode from 0600 to 0644 for readable log files.
  • Added/kept coverage for re-apply behavior with TestApplyOptionsToLoggersFileOutputReapply.
  • Fixed lint follow-ups from CI (noinlineerr, wsl_v5) and re-ran checks.

Latest commit with the lint follow-up: aa1dc88.

Could you please take another look when you have a moment?

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Open new log file before closing the old one so loggers are never
  left pointing at a closed FD if OpenFile fails
- Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent
  FD leaks on assertion failures

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
@nelson-parente
Copy link
Copy Markdown
Contributor

@JoshVanL The file handle is tracked and closed — see setLogOutput() in the latest commit (98aedc3):

  1. New file opens first
  2. All loggers switch to the new output
  3. Previous file closes after the switch
// Close the previous log file after loggers have been redirected.
if logOutputFile != nil {
    logOutputFile.Close()
}
logOutputFile = newFile

When OutputFile is empty (or on process shutdown via ApplyOptionsToLoggers with no path), loggers revert to stdout and the file is closed. The logOutputFile + logOutputMu mutex prevent FD leaks across re-applies.

nelson-parente added a commit to nelson-parente/kit that referenced this pull request Apr 9, 2026
Adds OutputFile option to logger and a --log-file CLI flag. When set,
all registered loggers write to the specified file instead of stdout.

- Mutex-protected package-level state tracks the active log file
- Atomic swap: new file opens before old one closes (no FD leak)
- Output reverts to stdout when OutputFile is empty

Backport of dapr#152 onto v0.16.1 for D3E release-1.16.
Tracks: OSS-1226

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
@JoshVanL JoshVanL merged commit 18c6fce into dapr:main Apr 9, 2026
6 checks passed
nelson-parente added a commit to nelson-parente/kit that referenced this pull request Apr 9, 2026
* feat: add OutputFile option for log destination in logger

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* feat: add file output support for logging

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* lint

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* add cleanup for file output logger in options tests

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* fix: address review feedback for log file output

- Relax file permissions from 0600 to 0644 to allow reads (per Albert's review)
- Track open log file handle in package-level state protected by mutex
  to prevent FD leaks when ApplyOptionsToLoggers is called multiple times
- Close previous log file before opening a new one on re-apply
- Revert output to stdout when OutputFile is empty (handles config reload)
- Extract setLogOutput helper for clarity
- Add TestApplyOptionsToLoggersFileOutputReapply test verifying
  re-apply closes the previous file and switches to the new one

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

* fix: improve error handling and output file management in logging

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* fix: address Copilot review — atomic file swap and early test cleanup

- Open new log file before closing the old one so loggers are never
  left pointing at a closed FD if OpenFile fails
- Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent
  FD leaks on assertion failures

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

---------

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Co-authored-by: Nelson Parente <nelson_parente@live.com.pt>
JoshVanL pushed a commit that referenced this pull request Apr 9, 2026
…elease-0.17 (#153)

* fix: support RSA and fix ed25519 key encoding in EncodePrivateKey (#148)

* fix: support RSA key encoding in EncodePrivateKey

EncodePrivateKey only handled *ecdsa.PrivateKey and ed25519.PrivateKey,
rejecting RSA keys with "unsupported key type". Since RSA keys also use
x509.MarshalPKCS8PrivateKey with the same PKCS#8 block type, add
*rsa.PrivateKey to the existing case list.

Adds table-driven tests covering RSA-2048, RSA-4096, ECDSA P-256,
Ed25519, and unsupported type with roundtrip verification through
DecodePEMPrivateKey.

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

* fix: address review feedback

- Drop RSA-4096 test case to avoid slow CI (2048 is sufficient coverage)
- Replace custom containsSubstr/searchSubstr with strings.Contains
- Remove unused helper functions

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

---------

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

* feat: add OutputFile option for log destination in logger (#152)

* feat: add OutputFile option for log destination in logger

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* feat: add file output support for logging

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* lint

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* add cleanup for file output logger in options tests

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* fix: address review feedback for log file output

- Relax file permissions from 0600 to 0644 to allow reads (per Albert's review)
- Track open log file handle in package-level state protected by mutex
  to prevent FD leaks when ApplyOptionsToLoggers is called multiple times
- Close previous log file before opening a new one on re-apply
- Revert output to stdout when OutputFile is empty (handles config reload)
- Extract setLogOutput helper for clarity
- Add TestApplyOptionsToLoggersFileOutputReapply test verifying
  re-apply closes the previous file and switches to the new one

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

* fix: improve error handling and output file management in logging

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>

* fix: address Copilot review — atomic file swap and early test cleanup

- Open new log file before closing the old one so loggers are never
  left pointing at a closed FD if OpenFile fails
- Register t.Cleanup immediately after ApplyOptionsToLoggers to prevent
  FD leaks on assertion failures

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>

---------

Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Co-authored-by: Nelson Parente <nelson_parente@live.com.pt>

---------

Signed-off-by: Nelson Parente <nelson_parente@live.com.pt>
Signed-off-by: MyMirelHub <15373565+MyMirelHub@users.noreply.github.com>
Co-authored-by: Mirel <15373565+MyMirelHub@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants