Skip to content

Improve logging

Afonso Mukai requested to merge improve_logging into master

Created by: SkyToGround

Description of work

Seeing as the logging code for the file writer (and forwarder) is somewhat confusing and hard to modify, I decided to spend a few hours improving the situation. My goal was to keep and improve upon the features that the existing logging implementation already had without regressing in functionality. This is what I have done:

  • Switched to the latest version of the graylog-logger library, made it required and updated the relevant CMake code.
  • Removed existing console and file logging functionality and switched over to using the corresponding implementation in graylog-logger.
  • Created a new kafka-to-graylog interface for the graylog-logger library to replace the existing kafka-to-graylog code.
  • Removed the enum class Sev type and replaced it with using Sev = Log::Severity.
  • Removed global variables defined in logger.cpp and fixed the rest of the code base to work without those.
  • Minor LLVM:ification of some of the options variable names.
  • Added support for both textual and numeric selection of verbosity/logging level in the command line interface.

When looking at this pull request, there are several important things to consider:

  • I found a serious issue in the existing code where log messages are self re-generating and thus are basically generated in an infinite loop. This should probably be fixed sooner rather than later.
  • Although I am a big proponent of "profile first, optimise later", it might be that some of my (possibly) worst pieces of code (in terms of performance) should be optimised before a potential merge. See if you can find them.
  • It might be appropriate to have some "trace" functionality for cases when you care about performance. I consider some of the LOG() calls wrapped in if-statements to be candidates for this.
  • Is there some obvious and important functionality that was lost when I modified the code?
  • I do not see it as a problem when making graylog-logger a required dependence for the file-writer (seeing as we are using Conan) but maybe there is a good argument against graylog-logger.

Issue

n/a

Unit Tests

n/a

Code Review (To be filled in by the reviewer only)

  • Is the code of an acceptable quality?
  • Do the changes function as described and is it robust?

Nominate for Group Code Review (Anyone can nominate it)

Indicate if you think the code should be reviewed in a Thursday code review session.

  • Recommend for group code review

Also, nominate it on the code_review Slack channel (does someone want to automate this?).

Merge request reports