-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 non-JSON log messages when using -log-format
JSON
#24252
Fix non-JSON log messages when using -log-format
JSON
#24252
Conversation
Removed the call to consul-template's logging.Setup inside the created of config for the Runner. Instead we call it when we assign the logger to the Agent command.
CI Results: |
I know this is a draft, but one idea I had for a test for this feature is that we could use the |
@@ -11,7 +11,6 @@ import ( | |||
ctconfig "github.com/hashicorp/consul-template/config" | |||
ctlogging "github.com/hashicorp/consul-template/logging" | |||
"github.com/hashicorp/go-hclog" | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nobody likes the extra line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if errors != nil { | ||
return nil, errors | ||
if errs != nil { | ||
return nil, errs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the error is of type multierror.Error, you may want to use errs.ErrorOrNil()
on the return.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @hghaf099, do you mean like this?
if errs.ErrorOrNil() != nil {
return nil, errs.ErrorOrNil()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, my bad. It seems that we only need to return if the error is non-nil. Then what you already had is sufficient. Sorry for the confusion.
c.logWriter = l.StandardWriter(&hclog.StandardLoggerOptions{ | ||
InferLevels: true, | ||
InferLevelsWithTimestamp: true, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the actual bug fix.
Thanks @VioletHynes, I'm not sure if that would work as the stdout/stderr is still used if file is enabled, you just end up writing to both. 🤔 |
Build Results: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I'd like to see a test before I approve, especially as this is dependent on consul-template
, I worry maybe we'll take a new version of that and this functionality would break.
I mentioned the option of checking stdout
in a test, but if it still has all of the output in tests even if it goes to a file, we could check that it has logs from consul-template
, e.g. check the log-file
for
"@message": "(runner) rendered "file-name"
There may be a smarter way to test this too, but that approach should be hopefully easy enough, and should test both aspects of the PR (log format and log file).
Co-authored-by: Violet Hynes <violet.hynes@hashicorp.com>
…hub.com:hashicorp/vault into peteski22/VAULT-17149/consul-template-logs-json
25373f2
to
88d6251
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of this PR is that log entries generated by the
consul-template
library used by Vault Agent will now be in a format consistent with the supplied configuration.Fixes: #21109
Steps to manually test:
make dev
)$HOME
directory (cd $HOME
)$HOME
directoryThe output should be entirely JSON, so you can pipe it through
jq
.Example content:
Example content: