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

Update go-plugin, and use hclog.Logger when serving a plugin #639

Merged
merged 2 commits into from
Dec 17, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Nov 6, 2020

Update go-plugin, so that it no longer overrides the default output in the log package.

Go-plugin can transfer JSON structured hclog output to a client logger. Rather than requiring existing providers to update all logging calls, we inject a wrapper into the std log package output which translates the defacto log format into structured log output with the correct levels. This will allow terraform to correctly filter the output, rather than default to handling all logs as Debug. This allows the logger to remove redundant tags and timestamps in the log output, and fixes multi-line log output where the first line may have a tag other than [DEBUG], but the remainder of the lines will be converted to [DEBUG].

This PR intends to only configure the logging when running as a plugin. The same logger could also be adapted for testing, but that is to be left for another PR.

The major fix here is go-plugin no longer overrides the default output
for the log package, allowing us to set it globally for providers.
Go-plugin can transfer JSON structured hclog output to a client logger.
Rather than requiring existing providers to udpate all logging calls, we
inject a wrapper into the std log package output which translates the
defacto log format into structured log output with the correct levels.
This will allow terraform to correctly filter the output, rather than
default to handling all logs as Debug.
Copy link
Contributor

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

Let's do it. Thanks @jbardin. I think we're going to want to follow up in the testing framework, but for the moment, we can ship this incremental improvement.

@paddycarver paddycarver merged commit 55bac52 into master Dec 17, 2020
@paddycarver paddycarver deleted the jbardin/hclog branch December 17, 2020 22:04
paddycarver added a commit that referenced this pull request Dec 18, 2020
Missed a logging override in #663. This should fix the last of the race
condition where logging can get written to stderr unexpectedly during
tests. We no longer need to do this because go-plugin was fixed and we
upgraded in #639. We don't want to overwrite that fix in tests.
paddycarver added a commit that referenced this pull request Dec 18, 2020
Missed a logging override in #663. This should fix the last of the race
condition where logging can get written to stderr unexpectedly during
tests. We no longer need to do this because go-plugin was fixed and we
upgraded in #639. We don't want to overwrite that fix in tests.
paddycarver added a commit that referenced this pull request Dec 18, 2020
When testing, we don't want to override where log output gets sent--the
test framework takes care of that itself. go-plugin has disabled its use
of log.SetOutput for us, but #639 introduced a log.SetOutput in
plugin.Serve. This adds a plugin.ServeOpt property to disable that
log.SetOutput call so we can finally have tests where log lines don't
randomly show up in test output.
paddycarver added a commit that referenced this pull request Dec 18, 2020
When testing, we don't want to override where log output gets sent--the
test framework takes care of that itself. go-plugin has disabled its use
of log.SetOutput for us, but #639 introduced a log.SetOutput in
plugin.Serve. This adds a plugin.ServeOpt property to disable that
log.SetOutput call so we can finally have tests where log lines don't
randomly show up in test output.
@ghost
Copy link

ghost commented Jan 17, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants