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.

Sign up for GitHub

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

Jump to bottom

Change default logging to INFO #84

Closed
wants to merge 2 commits into from
Closed

Change default logging to INFO #84

wants to merge 2 commits into from

Conversation

Mierdin
Copy link
Member

@Mierdin Mierdin commented Aug 23, 2017

Closes #77

@arm4b arm4b requested a review from humblearner August 23, 2017 20:12
@arm4b
Copy link
Member

arm4b commented Aug 23, 2017 •
edited

Could this potentially break any ChatOps CI or similar in our e2e tests @humblearner where we grep for specific log messages?
If so, we might need to inject debug loglevel in internal environment after this gets merged.

humblearner
humblearner suggested changes Aug 23, 2017
View reviewed changes
Copy link
Contributor

@humblearner humblearner left a comment •
edited

Choose a reason for hiding this comment

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

This will not only break chatopsCI, it will also break the self check script: https://github.com/StackStorm/st2chatops/blob/master/scripts/self-check.sh#L187 and it will add a step for new users, changing the debug level, who may have problem setting up st2chatops for the first time. It would be better to add step in docs to decrease verbosity.

@Mierdin
Copy link
Member Author

Mierdin commented Aug 23, 2017 •
edited

This change was actually requested by a user, that the messages are too verbose, and I'm inclined to agree - INFO is a common default log level. I'd prefer that we make this change, and follow @armab suggestion of injecting a DEBUG loglevel for our CI use case.

On a related note, the CircleCI checks have passed thus far, so if we're expecting CI to break, maybe something need adjusted.

@humblearner
Copy link
Contributor

@Mierdin: I have seen the conversation in #chatops community channel. I agree with you that the default log level should be info, but here it has been debug for a while, for a reason. Moreover, this is not related to CircleCI checks for this repo. This will cause failure in ChatopsCI (robot tests).

Please make changes at following locations as well:

  1. ChatOps CI in our e2e_tests

  2. Troubleshooting chatops for new users using this guide and self-check-script

However, I still don't agree with this change, because:

  • On one hand, as an advanced user, I can go ahead and change the log level in st2chatops.env file.
  • On the other, as a new user, I first change the log level to debug and then use the script. (Unless we make changes in the troubleshooting guide and the self-check script.)

Also, note, you will still see INFO logs every two minutes about the loaded commands:

Aug 23 19:31:24 st2demo004 hubot[8914]: [Wed Aug 23 2017 19:31:24 GMT+0000 (UTC)] INFO Connecting...
Aug 23 19:31:24 st2demo004 hubot[8914]: [Wed Aug 23 2017 19:31:24 GMT+0000 (UTC)] DEBUG Loading adapter slack
Aug 23 19:31:22 st2demo004 systemd[1]: Started StackStorm service st2chatops.
Aug 23 19:31:22 st2demo004 systemd[1]: Stopped StackStorm service st2chatops.
Aug 23 19:31:22 st2demo004 systemd[1]: Stopping StackStorm service st2chatops...
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] INFO 7 commands are loaded
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: demo reset diskspace on {{hostname}} - Reset diskspace demo
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: disk clean {{hostname}} {{directory=/var/log}} - Run diskspace
clean up from chatops
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: disk clean {{directory}} on {{hostname}} - Run diskspace clean
up from chatops
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack show {{ pack }} - Show information about the pack from Sta
ckStorm Exchange.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack show <pack> - Show information about the pack from StackSt
orm Exchange.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack search {{ query }} - Search for packs in StackStorm Exchan
ge and other directories.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack search <query> - Search for packs in StackStorm Exchange and other directories.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack install {{ packs }} - Install/upgrade StackStorm packs.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack install <gitUrl>[,<gitUrl>] - Install/upgrade StackStorm packs.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack install <pack>[,<pack>] - Install/upgrade StackStorm packs.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack get {{ pack }} - Get information about installed StackStorm pack.
Aug 23 19:31:17 st2demo004 hubot[29402]: [Wed Aug 23 2017 19:31:17 GMT+0000 (UTC)] DEBUG Added command: pack get <pack> - Get information about installed StackStorm pa:

@LindsayHill
Copy link
Contributor

Q: how many other apps ship with DEBUG logs on by default?

And if we are determined to ship with DEBUG, then surely we should at least sort out this other pack issue of no log rotate script?

@Mierdin
Copy link
Member Author

Mierdin commented Oct 24, 2018

Closing for now, since this is a bit stale. Feel free to pick up where I left off on the branch.

@Mierdin Mierdin closed this Oct 24, 2018
@arm4b arm4b reopened this Oct 25, 2018
@arm4b
Copy link
Member

arm4b commented Oct 25, 2018

The change itself is good. Maybe @blag can pick it up in future and update the st2tests as part of the initiative to rectify st2chatops.

cognifloyd
cognifloyd requested changes Nov 17, 2018
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

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

In order for the ansible smoke-test to succeed, it needs to be able to override this environment var. I suggest setting this to info only as a default if the var is not already set.

https://github.com/StackStorm/ansible-st2/blob/7dbcc49bc0adc3b2502b6f4caa07769f25fbf0b4/roles/st2smoketests/tasks/st2chatops.yml#L7-L8

st2chatops.env Outdated Show resolved Hide resolved
@arm4b arm4b self-assigned this Nov 17, 2018
@arm4b arm4b assigned arm4b and unassigned arm4b Nov 17, 2018
cognifloyd
cognifloyd approved these changes Nov 18, 2018
View reviewed changes
Copy link
Member

@cognifloyd cognifloyd left a comment •
edited

Choose a reason for hiding this comment

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

Looks good!

@cognifloyd
Copy link
Member

cognifloyd commented Nov 19, 2018 •
edited

re @humblearner's objection

  • On the other, as a new user, I first change the log level to debug and then use the script. (Unless we make changes in the troubleshooting guide and the self-check script.)

Maybe the self-check script (which I have not used or reviewed in detail) needs to modify the log level on the fly similar to how the ansible smoke-test sets the variable:
https://github.com/StackStorm/ansible-st2/blob/7dbcc49bc0adc3b2502b6f4caa07769f25fbf0b4/roles/st2smoketests/tasks/st2chatops.yml#L7-L8

@Mierdin Mierdin closed this Dec 5, 2021
@arm4b arm4b deleted the logging-info branch November 23, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arm4b arm4b arm4b left review comments

@humblearner humblearner humblearner requested changes

@cognifloyd cognifloyd cognifloyd approved these changes

@LindsayHill LindsayHill LindsayHill approved these changes

Assignees

@arm4b arm4b

Labels
breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Default log level should be INFO
5 participants
@Mierdin @arm4b @humblearner @LindsayHill @cognifloyd