search

IBM / ubiquity

Ubiquity
Apache License 2.0
90 stars 26 forks source link

Fix/ub 1325 change logger not to show pid and goroutine #218

Closed olgashtivelman closed 6 years ago

olgashtivelman commented 6 years ago

In this PR I wanted to update the new logging to the following state: in the ubiquity logger there we be shown the go routine id (and the pid will be removed from the log) : because the ubiquity server always runs in the same pid so no need to show it in the log since it does not give us any more information. in flex logger we will show the pid only (the go id routine will be removed) : since every flex request will always have the same go routine id there is not need to show both the request id and the go routine id. in provisioner log we will neither show the go routine id nor the pid of the process : since as for the flex logger each request will always have the same go routine id. and also like the ubiqity server the provisioner will always run with the same pid.

link to ubiquity-k8s PR : https://github.com/IBM/ubiquity-k8s/pull/194

This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.02%) to 51.501% when pulling 5c565c1e5ecf9d495fa0833188ca656d22e3446c on fix/UB-1325_change_logger_not_to_show_pid_and_goroutine into 7cbc7d31ff9bea6466753c02be6d0c386d4ba2a4 on dev.

shay-berman commented 6 years ago

Hi you did well, But I would like us to improve it for the same of simplicity. So we will not have to pass all the params every time we init the logger ubiqutiy server\flex and provisioner. Why ? because its hard for new developer to remember what are the relevant params needed for ubiqutiy\flex and provisioner (every ubiqutiy developer will have to read the description of this PR to understand it or just copy and past without understanding why).

So I want you to improve it a bit more for simplicity as follow:

  1. create ubiquityServerLogger which is with InitStdoutLogger with param of showGid=true, showpid=false. So from now on all the loggers in ubiquity server will use this one (no need to specify any args any more)
  2. create provisionerLogger (inside k8s repo) that actually uses InitStdoutLogger with param of showGid=true, showpid=false.
  3. create flexLogger (inside k8s repo) that actually uses InitFileLogger with param of showGid=false, showpid=true

So it will be very clear that for ubiquity server thing we just use ubiquityServerLogger with its own right configuration (right params and STDOUT) and for flex we use flexLogger with its own configuration and File logging and for provisioner we also have the provisioner logger.

What do you say Olga, does it make sense to you?

Review status: 0 of 14 files reviewed, all discussions resolved (waiting on @shay-berman)

Comments from Reviewable

olgashtivelman commented 6 years ago

@shay-berman I am not sure what is the benefit of creating new loggers. a new developer that is creating a new log (something that does not happen alot I guess (except for the flex we need to re-create for all commands - but guessing copy\pasting is good enough, we can make it into a function if this is the issue) and if someone is creating a new log for some new functionality they will probably want to set the params as they see fit, wont they? I am just not so sure why we need to maintain different loggers just for one if\else statement that differs them...

Review status: 0 of 14 files reviewed, all discussions resolved (waiting on @shay-berman)

Comments from Reviewable

olgashtivelman commented 6 years ago

@shay-berman please review the changes after out discussion

Review status: 0 of 15 files reviewed, all discussions resolved (waiting on @shay-berman)

Comments from Reviewable

shay-berman commented 6 years ago

Hi yes now its much cleaner, and you have flex\provisioner and ubiqutiy flex init log functions. So the logging logic for each component is in one place. Thanks

Added small comment to apply but more then that its

:lgtm:

Review status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)

database/suite_test.go, line 28 at r4 (raw file):

func TestDb(t *testing.T) {
  RegisterFailHandler(Fail)
  defer logs.InitStdoutLogger(logs.DEBUG, logs.LoggerParams{})()

here you go you can do another log function named UbiquityServerTestingLogger that will just call to logs.InitStdoutLogger(logs.DEBUG, logs.LoggerParams{})() - BTW i would state explicity that you don't want pid nor gid for unit testing (do we?).

Comments from Reviewable

olgashtivelman commented 6 years ago

Review status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @olgashtivelman and @shay-berman)

database/suite_test.go, line 28 at r4 (raw file):

Previously, shay-berman wrote…
here you go you can do another log function named UbiquityServerTestingLogger that will just call to logs.InitStdoutLogger(logs.DEBUG, logs.LoggerParams{})() - BTW i would state explicity that you don't want pid nor gid for unit testing (do we?).

the go-id in the unittests does not appear most of the time (not sure why probably the flow is not complete) so no reason to show it. the pid we dont want to show for the ubiquity server so I am guessing its not relevant here as well.

Comments from Reviewable

olgashtivelman commented 6 years ago

Review status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @shay-berman)

database/suite_test.go, line 28 at r4 (raw file):

Previously, olgashtivelman wrote…
the go-id in the unittests does not appear most of the time (not sure why probably the flow is not complete) so no reason to show it. the pid we dont want to show for the ubiquity server so I am guessing its not relevant here as well.

Done.

Comments from Reviewable

olgashtivelman commented 6 years ago

@shay-berman - I added the changes. will merge .

Review status: 0 of 15 files reviewed, 1 unresolved discussion (waiting on @shay-berman)

Comments from Reviewable