Added logging with levels. #51

cappe987 · 2021-10-01T20:49:44Z

Issue #50: reimplement logging.

Initialize it by setting the logging level you want displayed.
Create Log outputs with a level for when it should be displayed.
Set file output.

I didn't write any logging in the copying part since that will be affected by the Redis Pipeline change.

I set it to 3 output levels because I thought that seemed reasonable. Another option could be to have different categories, like Reads, Writes, Actions, etc.

DGKSK8LIFE

For now, just make logging.go a file within utils, no need for a separate directory (package) yet (though once utils grows enough I will do that).

Logging should be optional, and previously, was represented as a boolean key in the YAML config file (as log) as well as a field (Log) in the Config struct. I like your implementation of logging levels, but wondering if using ints to represent them over string args is less user-friendly.

cappe987 · 2021-10-03T20:03:39Z

Ah right. I forgot to add it to the config. I'll fix that.

I am using ints to represent levels because that's the natural way to represent levels. I used a "enum" to avoid accidentally inputting invalid logging levels (although Go doesn't support real enums so it's not completely safe anyways). Should we use strings like "one", "two", "three"? I also restricted it to 3 levels, which we could change or just remove any restriction on it.

As for putting it in its own dir/package is because I wanted it to have its own package. Not just be utils. like everything else. To be honest, I want to just completely change the name utils. Not everything can be considered "utilities". Give it proper package names. I'd personally use src or lib, but I saw Go likes to use pkg for the main code folder. Then use subfolders for any files so they can have packages that actually correspond to what they are.

pkg
  |--convert
  |   |--db.go
  |--logging
      |--logging.go
  |--utils
      |--validator.go

Even validator.go could have a proper package name but I couldn't think of one right now. Generally, you want to avoid having a bunch of utils functions. Try to find a proper name or place for them. Otherwise it becomes a loose collection of things that have nothing in common.

DGKSK8LIFE · 2021-10-03T20:42:21Z

Ah right. I forgot to add it to the config. I'll fix that.

I am using ints to represent levels because that's the natural way to represent levels. I used a "enum" to avoid accidentally inputting invalid logging levels (although Go doesn't support real enums so it's not completely safe anyways). Should we use strings like "one", "two", "three"? I also restricted it to 3 levels, which we could change or just remove any restriction on it.

As for putting it in its own dir/package is because I wanted it to have its own package. Not just be utils. like everything else. To be honest, I want to just completely change the name utils. Not everything can be considered "utilities". Give it proper package names. I'd personally use src or lib, but I saw Go likes to use pkg for the main code folder. Then use subfolders for any files so they can have packages that actually correspond to what they are.
pkg
  |--convert
  |   |--db.go
  |--logging
      |--logging.go
  |--utils
      |--validator.go
Even validator.go could have a proper package name but I couldn't think of one right now. Generally, you want to avoid having a bunch of utils functions. Try to find a proper name or place for them. Otherwise it becomes a loose collection of things that have nothing in common.

For logging, I was thinking of doing something similar to what I did for Postgres/MySQL detection in Convert (just doing a switch over the levels ex: "low", "medium", "high"), but honestly, an unsigned int Log field is probably easier, and more manageable than an Enum IMO.

In terms of rearranging utils, I agree 100%, just want to minimize overhead as much as possible but that shouldn't be an issue anyway. Also, for the Convert function, I'd prefer it be a method of Config because there's lots of parameter repetition and "DRY-violations" across the codebase; it's getting to the point where there are like 20+ chars running offscreen because of it.

Logging now uses uint32 for log level. Logging options added to config file. Re-organized files and code because otherwise it caused cyclic dependency when using the Config type for the `Convert` (now `copyTable`) function. DB opening moved out from `Convert` function. CopyToX functions no longer in the same files as Config.

cappe987 · 2021-10-04T17:38:38Z

Ended up having to do some re-organizing to use the Config struct in Convert. And while at it I decided to do a bit more organizing of code.

I changed logging to just use uint32 for level. Also added logging options to the config.

DGKSK8LIFE

I will merge once this is integrated with pipelining. I will have to examine this more deeply pre-merge. Also for logging levels, it's best to use uint8 because we don't need a higher range than that.

DGKSK8LIFE · 2021-10-11T19:34:49Z

utils/config.go

+	LogLevel    *uint8 `yaml:"log_level"`
 	LogFilename *string `yaml:"log_filename"`


Is there a reason why these field types have to be pointers?

DGKSK8LIFE

Looks good overall, please change the docs in README.md to reflect config changes. Also, I think we should figure out an algorithm to generate the optimal chunk size based on row count but that can be done later.

Accidentally used `fmt.Print` instead of `log.Print`. fmt won't use the logfile if that is set.

DGKSK8LIFE · 2021-10-11T22:23:04Z

Cool. Going to do some more testing on my end then will merge. Thanks

Added logging with levels.

d09a7ea

cappe987 added enhancement New feature or request Hacktoberfest labels Oct 1, 2021

DGKSK8LIFE requested changes Oct 3, 2021

View reviewed changes

DGKSK8LIFE added hacktoberfest-accepted performance and removed hacktoberfest-accepted labels Oct 3, 2021

cappe987 added 2 commits October 4, 2021 19:33

Removed redundant check.

fa50267

DGKSK8LIFE approved these changes Oct 4, 2021

View reviewed changes

cappe987 added 2 commits October 11, 2021 20:59

Added logging to insertion.

c6751dc

Changed log level to use uint8.

7f30aaf

DGKSK8LIFE reviewed Oct 11, 2021

View reviewed changes

cappe987 added 2 commits October 11, 2021 22:12

Update README.md

269fb68

Fixed minor mistake [See details]

45b3f86

Accidentally used `fmt.Print` instead of `log.Print`. fmt won't use the logfile if that is set.

DGKSK8LIFE merged commit cc3d189 into DGKSK8LIFE:unstable Oct 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added logging with levels. #51

Added logging with levels. #51

cappe987 commented Oct 1, 2021

DGKSK8LIFE left a comment •

edited

cappe987 commented Oct 3, 2021

DGKSK8LIFE commented Oct 3, 2021

cappe987 commented Oct 4, 2021

DGKSK8LIFE left a comment •

edited

DGKSK8LIFE Oct 11, 2021

DGKSK8LIFE left a comment •

edited

DGKSK8LIFE commented Oct 11, 2021

		LogLevel *uint8 `yaml:"log_level"`
		LogFilename *string `yaml:"log_filename"`

Added logging with levels. #51

Added logging with levels. #51

Conversation

cappe987 commented Oct 1, 2021

DGKSK8LIFE left a comment • edited

Choose a reason for hiding this comment

cappe987 commented Oct 3, 2021

DGKSK8LIFE commented Oct 3, 2021

cappe987 commented Oct 4, 2021

DGKSK8LIFE left a comment • edited

Choose a reason for hiding this comment

DGKSK8LIFE Oct 11, 2021

Choose a reason for hiding this comment

DGKSK8LIFE left a comment • edited

Choose a reason for hiding this comment

DGKSK8LIFE commented Oct 11, 2021

DGKSK8LIFE left a comment •

edited

DGKSK8LIFE left a comment •

edited

DGKSK8LIFE left a comment •

edited