Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Prettier logs #9

Open
nikhilsaraf opened this issue Sep 10, 2018 · 7 comments
Open

Prettier logs #9

nikhilsaraf opened this issue Sep 10, 2018 · 7 comments
Assignees
Labels
feature request New feature or request help wanted Extra attention is needed
Milestone

Comments

@nikhilsaraf
Copy link
Contributor

nikhilsaraf commented Sep 10, 2018

Desired Behavior

I want the logs to be more intuitive and easy to read.

Impact

The desired behavior will allow me to more easily gauge what the bot is doing and will also help with retrospective inspection of specific strategies.

Feature Suggestion

We can achieve the desired behavior by:

  1. having structured log lines at the sdex.go level that are well-documented.
  2. using a logging framework that supports colors:
@nikhilsaraf nikhilsaraf added this to the v1.0.0-rc3 milestone Sep 10, 2018
@nikhilsaraf nikhilsaraf added the feature request New feature or request label Sep 10, 2018
@nikhilsaraf nikhilsaraf modified the milestones: v1.0.0-rc4, v2.0.0 Oct 3, 2018
@nikhilsaraf nikhilsaraf added the help wanted Extra attention is needed label Nov 12, 2018
@Reidmcc
Copy link
Contributor

Reidmcc commented Nov 27, 2018

I could have a go at this one. Before I get started, what level of generalization are you thinking of?

I'm envisioning something like functions that format and return messages in a particular category in a consistent logging framework format. For example all log messages that are errors would be the same color and have the same metadata elements. Is that the right track?

@nikhilsaraf
Copy link
Contributor Author

nikhilsaraf commented Dec 1, 2018

@Reidmcc yes that's the right track.

You can start by creating a basic interface in the (new) support/logging package like so:

type Logger interface {
    // basic messages, appends a newline (\n) after each entry
    Info(msg string)

    // basic messages, can be custom formatted, similar to fmt.Printf. User needs to add a \n if they want a newline after the log entry
    Infof(msg string, args ...interface{})

    // error messages, indicates to the logger that these messages can be handled differently (different color, special format, email alerts, etc.). The type of logger will determine what to do with these messages. The logger should NOT panic on these messages. Appends a newline (\n) after each entry.
    Error(msg string)

    // error messages, indicates to the logger that these messages can be handled differently (different color, special format, email alerts, etc.). The type of logger will determine what to do with these messages. The logger should NOT panic on these messages. User needs to add a \n if they want a newline after the log entry.
    Errorf(msg string, args ...interface{})
}

It would be best to start with the following:

  • interface definition
  • 1 implementation with the current log type logger
  • converting over one file first, maybe trade.go.

We can handle structured logging later since that will be a specific implementation of this interface.


We should be able to nest loggers when creating them, example:

Logger logger := logging.BufferedLogger(
    logging.MakeMultiLogger(
        logging.MakeDateLogger(
            logging.MakeColoredLogger(
                logging.MakeFileLogger("myLogFile.log"),
                logging.ColorBlack,        // color for Info logs
                logging.ColorRed,          // color for Error logs
            )
        ),
        logging.MakeNetworkLogger("https://my-logging-url.com"),
    ),
    10,    // number of entries to buffer before sending to its inner logger
)

In the above example, the outermost logger will buffer up to 10 log entries before sending to its inner logger. The MultiLogger will send each entry to it's list of inner loggers. The DateLogger will prefix the date to each message before passing it on to its inner logger. The coloredLogger will modify the message so it is displayed with color. The file logger will log to a file called myLogFile.log. The second logger (passed to the multiLogger) will send the log lines to the network endpoint.

With the above interface and such a composable logger we can come up with a variety of different log setups with just a few basic components. The first component can be very basic which uses our existing log library.

The most important part in this task is to convert the existing log lines from Printf and Println statements to use the above interface.


The variable name for the logger object in each file should be standardized to logger so we can easily search for places in the code where we do not use this logger and use log instead.

Let me know if you have any questions.

@Reidmcc
Copy link
Contributor

Reidmcc commented Dec 1, 2018

Ok, sounds good. So right now there are just these steps:

  1. Set up the logger interface
  2. Switch all the log messages to use that interface
  3. Make sure it can still output to file

And specifically not to create any complex loggers like the web logger example.

@nikhilsaraf
Copy link
Contributor Author

yes, you got all that correct.

on the output to file, that will still use the current logic that you had added previously (i.e. we will not have a separate fileLogger and regular logger for the first implementation, just a LogLogger implementation).

@jmg421
Copy link

jmg421 commented Mar 4, 2019

I know nothing about how to do this sort of thing but am happy to help. I have kelp v1.4.0 installed on my Mac.

@nikhilsaraf
Copy link
Contributor Author

nikhilsaraf commented Mar 5, 2019

@jmg421 You should compile the latest version of Kelp from source, see the README for how to do this along with setting up the necessary tools. I'd recommend creating a new branch in your own fork for this project.

You can then start refactoring the log lines one file at a time from log.Printf to l.Infof and l.Errorf, and fatal errors to logger.Fatal. See a sample commit that does this here for the `cmd/trade.go file. You can then submit PRs against those changes to be included in the master branch. Please keep your Pull Requests to be for 1 PR per file. This will allow the code reviews to go quicker.

Let me know if you have any questions along the way!

@jmg421
Copy link

jmg421 commented Mar 6, 2019

Will do; need to get my environment set up first which will take a little while... i'm a complete n00b

nikhilsaraf pushed a commit to nikhilsaraf/kelp that referenced this issue Mar 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature request New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants