Skip to content
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.
This repository has been archived by the owner on Sep 26, 2022. It is now read-only.

Rework logs #165

Closed
sr-gi opened this issue Jul 2, 2020 · 5 comments
Closed

Rework logs #165

sr-gi opened this issue Jul 2, 2020 · 5 comments
Assignees
Milestone

Comments

@sr-gi
Copy link
Member

sr-gi commented Jul 2, 2020

Currently logs are split in console plus file:

console logs are human readable, whereas file logs are in json. The purpose of the latter was to use it to send data to elasticsearch, but we've currently discussed that it may be worth doing that online instead of using an intermediary file (#148).

Ultimately, I think we should have 3 types of log handlers:

  • console
  • file
  • elasticsearch

The first is exactly as is, logs are displayed in a terminal in human readable format. The second should be the same as the first, but saving them in the log file. Finally, the latter should be based on the format of the current file log, but instead of saving data into a file, sending it to the class in charge to send data to the system monitor.

My intuition is that the default should be console + file logging, allowing a silent (or daemon) option to not show data in a terminal, and also having options to opt-in with the system monitor.

@sr-gi
Copy link
Member Author

sr-gi commented Jul 2, 2020

Pinging @orbitalturtle since you're currently working on the system monitor and @bigspider since you may start working or reworking the logs.

Also @aljazceru since we've had some conversations regarding Python logging, it may be worth having a discussion of what's the best way of handling Python logs (wether to handle it within the code or leave it to systemd or similar)

@bigspider
Copy link
Collaborator

Planning to work on this tomorrow morning.

@bigspider bigspider self-assigned this Jul 3, 2020
@bigspider
Copy link
Collaborator

bigspider commented Jul 3, 2020

After doing some research on python's logging module, while there are quite a lot of advanced features, there's no easy way of supporting structured messages (e.g.: message + a dict of additional fields), which is indeed why we have our Logger class in the first place.
Rather than reinventing the wheel, I think that structlog is a mature option that has all the feature we might need. The basic interface of its logger is compatible with our current Logger class, and it also supports "child loggers" where you bind a specific value that does not need to be passed everytime (like we do for actor field). Since it's a wrapper on python's logging library, we don't lose access to more advanced features that are available there.
Moreover, there are examples on how to combine it with the elasticsearch/logstash/kibana (for exaple here), so should be easy to get it to work with it once #148 is merged.

Gonna try to rewrite the logger using structlog and see how it turns out.

@aljazceru
Copy link
Contributor

i'd suggest not directly forcing people to use elk stack since people use various solutions for log aggregation. But having structured output or at least easily grok-able logs goes a long way to making it simple for people to fit it into their systems

As far as python filehandlers go, using https://docs.python.org/3/library/logging.handlers.html#logging.handlers.WatchedFileHandler make things smooth as you can manipulate it with external tools without having to deal with open filehandlers.

@bigspider
Copy link
Collaborator

In #168 I do some work that I think is in the right direction. It keeps the logging as console+file as it is now, but it adds the structlog wrapper that allows to keep the log entries as a dict until we format them for the standard logging system (which still uses WatchedFileHandler for the file output, so hopefully it makes you happy, too :))

It would be fairly easy at this point to support different format for the logs (e.g. have an option to change the log format to json), and also to integrate the elk stack (which anyway can always be kept as an opt-in feature).

@sr-gi sr-gi added this to the v0.0.1 milestone Jul 14, 2020
@sr-gi sr-gi closed this as completed Jul 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants