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

log: support for user defined log levels #3380

Closed
wants to merge 2 commits into from
Closed

log: support for user defined log levels #3380

wants to merge 2 commits into from

Conversation

fatih
Copy link
Contributor

@fatih fatih commented Oct 1, 2015

This PR brings support for better log level handling. Terraform logs
are already written in the form which
https://github.com/hashicorp/logutils can understand easily. Based on
this fact we know have the followings:

  • A new TF_LOG_LEVEL environment variable which the user can control
  • Users can pass levels in the form of "info", "Info" or "INFO"
  • If an invalid log level is passed, we print a warning but still
    continue
  • All changes are backwards compatible with the current logs

Some pictures:

Without any changes:

screen shot on 2015-10-01 at 22_49_14

With TF_LOG_LEVEL=info:

screen shot on 2015-10-01 at 22_51_04

With a wrong log level:

screen shot 2015-10-01 at 22 53 00

This PR brings support for better log level handling. Terraform logs
are already written in the form which
https://github.com/hashicorp/logutils can understand easily. Based on
this fact we know have the followings:

* A new `TF_LOG_LEVEL` environment variable which the user can control
* Users can pass levels in the form of "info", "Info" or "INFO"
* If an invalid log level is passed, we print a warning but still
  continue
* All changes are backwards compatible with the current logs
@apparentlymart
Copy link
Contributor

Thanks for working on this! It seems like it'd be very useful, since the existing logs are incredibly verbose. (I'm sure that this change would expose some cases where existing logs may be better changed to a new level, or have a level added; e.g. detecting your home directory is arguably no more than DEBUG.)

In terms of the "user-interface" for this, it seems a little awkward that a user must set both TF_LOG and TF_LOG_LEVEL. I wonder if instead we could say that TF_LOG itself now takes the various log levels as values, so you can just say (for example) TF_LOG=WARNING and have that both turn on logging and set the filter all at once.

For compatibility with existing usage patterns the values 1, true and yes could be treated as undocumented aliases for TRACE, but any new docs would say to set TF_LOG=TRACE in order to see all of the logs. (I'd also keep your behavior of warning but continuing with no filter when an invalid level is selected, to take care of any other existing users setting TF_LOG to other non-empty values.)

How does that sound?

@fatih
Copy link
Contributor Author

fatih commented Oct 4, 2015

Hi @apparentlymart

In terms of the "user-interface" for this, it seems a little awkward that a user must set both TF_LOG and TF_LOG_LEVEL. I wonder if instead we could say that TF_LOG itself now takes the various log levels as values, so you can just say (for example) TF_LOG=WARNING and have that both turn on logging and set the filter all at once.

You are right, I just wanted to break the functionality. Adding an option without touching the first one seems to be ok for me. But your approach is just fine and even better :) I'll update the PR with the changes.

Having only one environment level makes it more easier to use.
@fatih
Copy link
Contributor Author

fatih commented Oct 5, 2015

I've updated it and the current usage is like the following:

Using it with info:

image

Old usage. It's still backwards compatible, though we display an error message for a proper usage:

image

@apparentlymart
Copy link
Contributor

Thanks @fatih! That looks awesome.

Since this is a change to the interface I'm not going to just merge this in right now since I want to give other folks a chance to chime in. @phinze does this look good to you?

@apparentlymart
Copy link
Contributor

I rebased this and merged it as 0090c06. Thanks for this. It should make debugging easier if we can hide all of those noisy TRACE lines!

@ghost
Copy link

ghost commented Apr 30, 2020

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 and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants