[Review please!] Logging refactor / cleanup by cdrage · Pull Request #415 · projectatomic/atomicapp

cdrage · 2015-11-23T17:21:20Z

This is a work-in-progress.

It currently looks like this:

DONE!

Review away :)

landscape-bot · 2015-11-23T17:25:49Z

Repository health increased by 2% when pulling d6c9d1e on cdrage:new-logging into 5504ddf on projectatomic:master.

9 new problems were found (including 0 errors and 9 code smells).
13 problems were fixed (including 0 errors and 13 code smells).

landscape-bot · 2015-11-23T17:35:53Z

Repository health increased by 2% when pulling 19e2aaf on cdrage:new-logging into 5504ddf on projectatomic:master.

9 new problems were found (including 0 errors and 9 code smells).
13 problems were fixed (including 0 errors and 13 code smells).

dustymabe · 2015-11-23T20:34:23Z

Should the [OK] messages in your picture of example output be [INFO] instead?

landscape-bot · 2015-11-24T00:16:16Z

Repository health increased by 2% when pulling f0f21e2 on cdrage:new-logging into 5504ddf on projectatomic:master.

10 new problems were found (including 0 errors and 9 code smells).
13 problems were fixed (including 0 errors and 13 code smells).

cdrage · 2015-11-24T01:13:19Z

@dustymabe Yeah, it should :), I'll fix that.

landscape-bot · 2015-11-24T02:56:16Z

Repository health increased by 2% when pulling 3434eaa on cdrage:new-logging into 5504ddf on projectatomic:master.

5 new problems were found (including 0 errors and 5 code smells).
13 problems were fixed (including 0 errors and 13 code smells).

sub-mod · 2015-12-11T18:22:59Z

@cdrage
All the display messages cannot be shown on UI. I would like a method where I can add cockpit specific message like display("step 1 completed", cockpit=true) and these messages should show up when i put cockpit flag

also, can journald output be another PR so that we can have some code for me to test with cockpit

cdrage · 2015-12-11T18:43:54Z

Yup. JournalD will be another PR. At the moment its not possible.

I'll update the PR and add a function for cockpit only info messages so you
can at least test it with cockpit.
On Dec 11, 2015 1:23 PM, "sub-mod" notifications@github.com wrote:

@cdrage https://github.com/cdrage
All the display messages cannot be shown on UI. I would like a method
where I can add cockpit specific message like display("step 1 completed",
cockpit=true) and these messages should show up when i put cockpit flag
also, can journald output be another PR so that we can have some code for
me to test with cockpit
—
Reply to this email directly or view it on GitHub
#415 (comment)
.

dustymabe · 2016-02-17T19:33:27Z

+ return input
+
+
+def set_logging(args):


Let's modify this to take vars as input. Looks like the few that we use are: verbose, quiet, and logging_output.
That way we aren't coupled to changes in cli/main.py.

cdrage · 2016-02-17T23:28:56Z

Okay so I've gone ahead and pushed all my other commits.

I have one failing error right now that could be due to a bug in the test suite. Otherwise, good for another review!

dustymabe · 2016-02-18T04:24:08Z

+ return
+
+ if code == LOG_LEVELS['debug']:
+ msg = "[DEBUG] %6d %0.2f %s" % (os.getpid(), time.time(), msg)


After running it a couple of times I feel like the extra output in the debug statements (the pid/time) doesn't really help debug anything any more. We don't run multiprocess so I don't think this is useful. Thoughts?

@dustymabe updated. yeah, too much output!

dustymabe · 2016-02-18T04:27:55Z

So one really nice thing (for me) about the old output is that it included the name of the file in the output:

[root@f23 redis-centos7-atomicapp (upstreammaster *%=)]# aa run ./ --provider kubernetes 2016-02-17 20:37:21,499 - __main__ - INFO - Action/Mode Selected is: run
2016-02-17 20:37:21,532 - atomicapp.plugin - WARNING - Configuration option 'providerconfig' not found
2016-02-17 20:37:21,532 - atomicapp.plugin - WARNING - Configuration option 'providerconfig' not found
2016-02-17 20:37:21,532 - kubernetes - INFO - Using namespace default
2016-02-17 20:37:21,532 - kubernetes - INFO - trying kubectl at /host/usr/bin/kubectl
2016-02-17 20:37:21,533 - kubernetes - INFO - found kubectl at /host/usr/bin/kubectl
2016-02-17 20:37:21,535 - kubernetes - INFO - Deploying to Kubernetes
2016-02-17 20:37:21,808 - atomicapp.plugin - WARNING - Configuration option 'providerconfig' not found
2016-02-17 20:37:21,808 - kubernetes - INFO - Using namespace default
2016-02-17 20:37:21,809 - kubernetes - INFO - trying kubectl at /host/usr/bin/kubectl
2016-02-17 20:37:21,809 - kubernetes - INFO - found kubectl at /host/usr/bin/kubectl
2016-02-17 20:37:21,810 - kubernetes - INFO - Deploying to Kubernetes

Is there a way to preserve that with the new system?

cdrage · 2016-02-18T14:57:25Z

@rtnpro @dustymabe

Hey all!

So I've updated the PR for final review.

One thing to note is that I will be removing the syslog logging output provider until a later date. I want to do more thorough testing with that.

I've also had to move a few things around in main.py in order to get the logging to output correctly. I checked to see if it would do any harm and from the looks if it, it wouldn't.

Otherwise, feel free to review and test!

dustymabe · 2016-02-18T15:18:25Z

+ custom_level = LOG_LEVELS[logging_output]
+ break
+ else:
+ custom_level = LOG_LEVELS['default']


LOG_LEVELS is a list right? could we do something like this:
if logging_output in LOG_LEVELS: custom_level = LOG_LEVELS[logging_output] else: custom_level = LOG_LEVELS['default']

@dustymabe Ahhhhh, forgot Python had that functinon >.> I'll add it

cdrage · 2016-02-18T16:17:47Z

@dustymabe updated based on your comments for the for loop

dustymabe · 2016-02-19T18:54:17Z

Closing in favor of #565

cdrage force-pushed the new-logging branch 2 times, most recently from 1da2472 to d6c9d1eCompare November 23, 2015 17:24

cdrage closed this Nov 23, 2015

cdrage force-pushed the new-logging branch from d6c9d1e to e362bb8Compare November 23, 2015 17:27

cdrage reopened this Nov 23, 2015

cdrage added the work in progress label Nov 23, 2015

rtnpro added the GA label Nov 25, 2015

rtnpro added this to the CDK 2 GA milestone Nov 25, 2015

rtnpro removed the GA label Nov 25, 2015

cdrage mentioned this pull request Nov 25, 2015
Non-quiet, non-verbose mode should output pretty strings #186
Closed

cdrage force-pushed the new-logging branch 8 times, most recently from 1ca21f2 to ffd768aCompare December 2, 2015 23:38

sub-mod mentioned this pull request Dec 8, 2015
Fix cockpit integration #432
Closed

goern assigned cdrage Dec 11, 2015

cdrage force-pushed the new-logging branch 2 times, most recently from 0cc39f4 to 9b7b496Compare December 21, 2015 17:21

cdrage force-pushed the new-logging branch 2 times, most recently from 1bcee57 to a9ebb56Compare February 17, 2016 18:33

dustymabe reviewed Feb 17, 2016
View reviewed changes

cdrage force-pushed the new-logging branch 2 times, most recently from 8172d95 to 3edb100Compare February 17, 2016 23:27

dustymabe reviewed Feb 18, 2016
View reviewed changes

cdrage force-pushed the new-logging branch from 067a54b to 87661daCompare February 18, 2016 14:45

cdrage changed the title ~~[WIP] Logging refactor / cleanup!~~[Review please!] Logging refactor / cleanupFeb 18, 2016

cdrage removed the work in progress label Feb 18, 2016

cdrage force-pushed the new-logging branch 2 times, most recently from e36e4d0 to e5b9b43Compare February 18, 2016 14:56

dustymabe reviewed Feb 18, 2016
View reviewed changes

cdrage force-pushed the new-logging branch from e5b9b43 to fd2e945Compare February 18, 2016 16:10

cdrage added 4 commits February 18, 2016 11:17

Add display class integration
4c92a5a

Cockpit logging output support
d47d98b

Convert logging library outputs to display class
66a8e28

Fix test on errornous output
282f07b

cdrage force-pushed the new-logging branch from fd2e945 to 282f07bCompare February 18, 2016 16:19

dustymabe closed this Feb 19, 2016

cdrage deleted the new-logging branch February 21, 2016 01:11

Uh oh!

Conversation

cdrage commented Nov 23, 2015

DONE!

Uh oh!

landscape-bot commented Nov 23, 2015

Uh oh!

landscape-bot commented Nov 23, 2015

Uh oh!

dustymabe commented Nov 23, 2015

Uh oh!

landscape-bot commented Nov 24, 2015

Uh oh!

cdrage commented Nov 24, 2015

Uh oh!

landscape-bot commented Nov 24, 2015

Uh oh!

sub-mod commented Dec 11, 2015

Uh oh!

cdrage commented Dec 11, 2015

Uh oh!

dustymabeFeb 17, 2016

Choose a reason for hiding this comment

Uh oh!

cdrage commented Feb 17, 2016

Uh oh!

dustymabeFeb 18, 2016

Choose a reason for hiding this comment

Uh oh!

cdrageFeb 18, 2016

Choose a reason for hiding this comment

Uh oh!

dustymabe commented Feb 18, 2016

Uh oh!

cdrage commented Feb 18, 2016

Uh oh!

dustymabeFeb 18, 2016

Choose a reason for hiding this comment

Uh oh!

cdrageFeb 18, 2016

Choose a reason for hiding this comment

Uh oh!

cdrage commented Feb 18, 2016

Uh oh!

dustymabe commented Feb 19, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants