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

agent: remove leading whitespace from agent log lines #10338

Merged
merged 6 commits into from
Jun 3, 2021

Conversation

dnephin
Copy link
Contributor

@dnephin dnephin commented Jun 2, 2021

This PR fixes a problem reported by a user:

fluentd is considering the logs (rom Consul pods as multiline and results in combining (all) the logs.

Every log line had 4 spaces prepended. This was caused by the UI interface that is used in the CLI. We have encountered a number of problems with the UI interface. This PR takes the first step to fix the interface by exposing the underlying Stdout/Stderr streams as io.Writer.

The PR includes some other cleanup, and is best viewed by individual commit.

I tested this by running consul agent -dev and consul agent -dev -log-json to verify it worked as it did before, with the exception of the leading spaces and other small improvements which I have commented on below.

The version args are static and passed in from the caller. Instead read
the static values in New.

The shutdownCh was never closed, so did nothing. Remove it as a field
and an arg.
This will allow commands to do the right thing, and write to the proper
output stream.
Remove the leading whitespace on every log line. This was causing problems for
a customer because their logging system was interpretting the logs as a single
multi-line log.
Previously this line was mixed up with logging, which made the output
quite ugly. Use the logger to output this message, instead of printing
directly to stdout.

This has the advantage that the message will be visible when json logs
are enabled.
The intent of this struct was to prevent non-json output to stdout. With
the previous cleanup, this can now be done by simply changing the stdout
stream to io.Discard.

This is just one example of why passing around io.Writers for the
streams is better than the UI interface.
@dnephin dnephin requested a review from a team June 2, 2021 21:34
@github-actions github-actions bot added the theme/cli Flags and documentation for the CLI interface label Jun 2, 2021
Comment on lines +161 to +163
// FIXME: logs should always go to stderr, but previously they were sent to
// stdout, so continue to use Stdout for now, and fix this in a future release.
logGate := &logging.GatedWriter{Writer: c.ui.Stdout()}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the fix for the primary issue, but other changes were required to make this possible.

@@ -119,7 +113,7 @@ func (c *cmd) startupJoin(agent *agent.Agent, cfg *config.RuntimeConfig) error {
return nil
}

c.UI.Output("Joining cluster...")
c.logger.Info("Joining cluster")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change to use a logger (and mentioned in the commit message). Using the logger for this seems much better, that way the output is not lost when json logging is enabled.

Same below.

return 1
}

// Let the agent know we've finished registration
agent.StartSync()

cli.output("Consul agent running!")
c.logger.Info("Consul agent running!")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also changed to use the logger (as mentioned in the commit message). Previously this was leading to very odd looking output, where the log lines were broken up by a random ==> Consul agent running!" line.

With this change the message will be included in the logs when json logs are enabled.

Comment on lines -284 to -285
case <-c.shutdownCh:
sig = os.Interrupt
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed because it did nothing (the channel was never closed), and we already have a case for handling signals.

@vercel vercel bot temporarily deployed to Preview – consul June 2, 2021 21:40 Inactive
@vercel vercel bot temporarily deployed to Preview – consul-ui-staging June 2, 2021 21:40 Inactive
Copy link
Member

@rboyer rboyer left a comment

Choose a reason for hiding this comment

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

LGTM

@dnephin dnephin merged commit f204daf into master Jun 3, 2021
@dnephin dnephin deleted the dnephin/fix-logging-indent branch June 3, 2021 16:16
@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/380243.

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

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

hc-github-team-consul-core pushed a commit that referenced this pull request Jun 3, 2021
agent: remove leading whitespace from agent log lines
@hc-github-team-consul-core
Copy link
Collaborator

🍒❌ Cherry pick of commit f204daf onto release/1.9.x failed! Build Log

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

🍒❌ Cherry pick of commit f204daf onto release/1.8.x failed! Build Log

dnephin added a commit that referenced this pull request Jun 3, 2021
agent: remove leading whitespace from agent log lines
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants