refactor: Remove extra log levels #634

fredcarle · 2022-07-14T00:01:29Z

Relevant issue(s)

Resolves #612

Description

This PR removes Warn, FeedbackWarn and FeedbackDebug. It was initially going to remove Fatal and FatalE as well but other out of scope changes need to be done prior to considering this.

Keeping just this minor reduction for now and will revisit for v0.3.1.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Unit testing

Specify the platform(s) on which this was tested:

MacOS

codecov · 2022-07-14T00:02:11Z

Codecov Report

Merging #634 (eab50ad) into develop (303b43a) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head eab50ad differs from pull request most recent head 0ccf822. Consider uploading reports for the commit 0ccf822 to get more accurate results

@@             Coverage Diff             @@
##           develop     #634      +/-   ##
===========================================
- Coverage    57.00%   56.96%   -0.05%     
===========================================
  Files          122      122              
  Lines        14661    14646      -15     
===========================================
- Hits          8358     8343      -15     
  Misses        5588     5588              
  Partials       715      715

Impacted Files	Coverage Δ
logging/logger.go	`76.35% <ø> (-2.18%)`	⬇️
logging/logging.go	`100.00% <ø> (ø)`
net/peer.go	`12.54% <0.00%> (ø)`

orpheuslummis · 2022-07-14T00:12:48Z

It'd be be nice to communicate to zap the exact levels we're using, instead of it internally using its default 7, but I could only find a way to add more levels

orpheuslummis · 2022-07-14T00:18:47Z

net/peer.go

@@ -462,7 +462,7 @@ func stopGRPCServer(ctx context.Context, server *grpc.Server) {
 	select {
 	case <-timer.C:
 		server.Stop()
-		log.Warn(ctx, "Peer gRPC server was shutdown ungracefully")
+		log.Info(ctx, "Peer gRPC server was shutdown ungracefully")


Seems like an Error

To me this is not an error. An error is something that needs to be handled by the caller. This is just a message sent back to the console/logs to let the user know that something happened. Hence it's just Information.

Do we want to prepend something like "Warning" on some of the Info messages? I'm towards yes.

Do we want to prepend something like "Warning" on some of the Info messages? I'm towards yes.

🤣 Please dont do this lol. If someone really wants it to be a warning then they should use/re-introduce it as a log level (I dont think they should lol, but it is better than doing this)

orpheuslummis · 2022-07-14T00:21:59Z

A) needs to remove in config as well:

defradb/config/config.go

Line 359 in eab50ad

case "warn":

B) why remove FeedbackDebug ?

C) other instances ...

defradb/cli/root.go

Line 96 in eab50ad

"log level to use. Options are debug, info, warn, error, fatal",

defradb/config/config_test.go

Line 150 in eab50ad

assert.Equal(t, "warn", cfg.Logging.Level)

defradb/config/configfile.go

Line 85 in eab50ad

# Log level. Options are debug, info, warn, error, fatal

D) should this be modified as well?

defradb/logging/config.go

Line 56 in eab50ad

const (

along with

defradb/logging/logging_test.go

Line 207 in eab50ad

return []LogLevelTestCase{

fredcarle · 2022-07-14T00:50:43Z

A, C and D yes. Didn't think about those.

B, FeedbackDebug makes no sense to me. Debug is a message for Defra devs. Hence the logs will not be sent to file if we are doing development. And even if they are, we're looking at them. Feedback is for users. Not Devra devs.

fredcarle · 2022-07-14T00:52:29Z

It'd be be nice to communicate to zap the exact levels we're using, instead of it internally using its default 7, but I could only find a way to add more levels

Does it matter though? We are pretty much doing that by only exposing the log levels we want.

orpheuslummis · 2022-07-14T10:20:04Z

It'd be be nice to communicate to zap the exact levels we're using, instead of it internally using its default 7, but I could only find a way to add more levels

Does it matter though? We are pretty much doing that by only exposing the log levels we want.

With that zap would do slightly less internal work, but it doesn't matter too much, and it a quick look into it led me to guess that lowering levels is not possible with zap anyway.

AndrewSisley

LGTM - cheers Fred

orpheuslummis

Approved conditional on A, C and D discussed above being fixed.

Relevant issue(s) Resolves sourcenetwork#612 Description This PR removes Warn, FeedbackWarn and FeedbackDebug. It was initially going to remove Fatal and FatalE as well but other out of scope changes need to be done prior to considering this. Keeping just this minor reduction for now and will revisit for v0.3.1.

remove extra log levels

eab50ad

fredcarle added refactor This issue specific to or requires *notable* refactoring of existing codebases and components area/logging Related to the logging/logger system action/no-benchmark Skips the action that runs the benchmark. labels Jul 14, 2022

fredcarle added this to the DefraDB v0.3 milestone Jul 14, 2022

fredcarle requested a review from a team July 14, 2022 00:01

fredcarle self-assigned this Jul 14, 2022

orpheuslummis reviewed Jul 14, 2022

View reviewed changes

AndrewSisley approved these changes Jul 14, 2022

View reviewed changes

orpheuslummis approved these changes Jul 14, 2022

View reviewed changes

remove leftover warn related code

0ccf822

fredcarle merged commit 637fc44 into develop Jul 14, 2022

fredcarle deleted the fredcarle/refactor/I612-reduce-logging-levels branch July 14, 2022 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Remove extra log levels #634

refactor: Remove extra log levels #634

fredcarle commented Jul 14, 2022

codecov bot commented Jul 14, 2022 •

edited

Loading

orpheuslummis commented Jul 14, 2022 •

edited

Loading

orpheuslummis Jul 14, 2022

fredcarle Jul 14, 2022

orpheuslummis Jul 14, 2022 •

edited

Loading

AndrewSisley Jul 14, 2022

orpheuslummis commented Jul 14, 2022 •

edited

Loading

fredcarle commented Jul 14, 2022

fredcarle commented Jul 14, 2022

orpheuslummis commented Jul 14, 2022

AndrewSisley left a comment

orpheuslummis left a comment

refactor: Remove extra log levels #634

refactor: Remove extra log levels #634

Conversation

fredcarle commented Jul 14, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Jul 14, 2022 • edited Loading

Codecov Report

orpheuslummis commented Jul 14, 2022 • edited Loading

orpheuslummis Jul 14, 2022

Choose a reason for hiding this comment

fredcarle Jul 14, 2022

Choose a reason for hiding this comment

orpheuslummis Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

AndrewSisley Jul 14, 2022

Choose a reason for hiding this comment

orpheuslummis commented Jul 14, 2022 • edited Loading

fredcarle commented Jul 14, 2022

fredcarle commented Jul 14, 2022

orpheuslummis commented Jul 14, 2022

AndrewSisley left a comment

Choose a reason for hiding this comment

orpheuslummis left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 14, 2022 •

edited

Loading

orpheuslummis commented Jul 14, 2022 •

edited

Loading

orpheuslummis Jul 14, 2022 •

edited

Loading

orpheuslummis commented Jul 14, 2022 •

edited

Loading