Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Windows logging to files #11960

Merged
merged 4 commits into from Jan 6, 2022
Merged

Fix Windows logging to files #11960

merged 4 commits into from Jan 6, 2022

Conversation

kisunji
Copy link
Contributor

@kisunji kisunji commented Jan 6, 2022

closes #11773

Our logger uses io.MultiWriter which at minimum writes to os.Stdout and optionally to syslog or log file targets.

When Consul is run as a Windows Service, os.Stdout is not available and any writes by the logger fails silently, which inadvertently stopped writes to other targets because of io.MultiWriter's behaviour:

If a listed writer returns an error, that overall write operation stops 
and returns the error; it does not continue down the list.

This PR uses a noErrorWriter wrapper which never returns an error so that even if writes to Stdout fail, writes to syslog and log file are unaffected.

@github-actions github-actions bot added theme/cli Flags and documentation for the CLI interface theme/telemetry Anything related to telemetry or observability labels Jan 6, 2022
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice fix!

UIWriter also strips trailing newlines, I wonder if that might cause other problems.

Would this also work?

type noErrorWriter struct {
    w io.Writer
}

func (w noErrorWriter) Write(p []byte) (n int, err error) {
    n, _ := w.w.Write(p)
    return n, nil
}

Edit: Oh, I just remembered b4b85bd, which did the opposite of this PR to fix a different bug. So I think we should use something like noErrorWriter to prevent re-introducing that bug. ui.Info is not what we want, we just want to drop any errors.

@vercel vercel bot temporarily deployed to Preview – consul January 6, 2022 20:33 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 6, 2022 20:33 Inactive
@kisunji kisunji force-pushed the kisunji/fix-windows-logging branch from 794bf4b to 2917d0e Compare January 6, 2022 20:37
@vercel vercel bot temporarily deployed to Preview – consul January 6, 2022 20:37 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 6, 2022 20:37 Inactive
@kisunji kisunji requested a review from dnephin January 6, 2022 20:38
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for debugging and fixing this!

@kisunji
Copy link
Contributor Author

kisunji commented Jan 6, 2022

Testing on windows:
image

@vercel vercel bot temporarily deployed to Preview – consul January 6, 2022 20:50 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging January 6, 2022 20:50 Inactive
@kisunji kisunji merged commit 08af4f7 into main Jan 6, 2022
@kisunji kisunji deleted the kisunji/fix-windows-logging branch January 6, 2022 21:07
@hc-github-team-consul-core
Copy link
Collaborator

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/542579.

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 08af4f7 onto release/1.11.x succeeded!

@hc-github-team-consul-core
Copy link
Collaborator

🍒✅ Cherry pick of commit 08af4f7 onto release/1.10.x succeeded!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme/cli Flags and documentation for the CLI interface theme/telemetry Anything related to telemetry or observability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

consul agent logs not works properly on windows (empty file)
3 participants