Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

2.8.x allow external logging formatters #836

Closed

Conversation

raoulvm
Copy link

@raoulvm raoulvm commented Aug 25, 2022

Proposed changes:
To be able to use structured logging, there has to be a dedicated method to define the logger for the Rasa SDK server:

  • introduce an environment variable RASA_LOGGER_PYMODULE to store the python package name of the custom logger
  • catch the previously uncaught exceptions from route handlers, so the exception output is also logged and not written to stderr in plain text.

Background: we use GrayLog with JSON extractor for all our servers, and

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog (please check changelog for instructions)
  • reformat files using black (please check Readme for instructions)

@CLAassistant
Copy link

CLAassistant commented Aug 25, 2022

CLA assistant check
All committers have signed the CLA.

@raoulvm
Copy link
Author

raoulvm commented Aug 25, 2022

@tmbo Here the PR I mentioned during the call on Tuesday.

@tmbo tmbo self-requested a review August 26, 2022 08:13
@tmbo
Copy link
Member

tmbo commented Aug 30, 2022

@raoulvm I've taken a closer look and ideally I'd like to aim at a unified solution across our different product parts (SDK, Rasa Open Source, Rasa Enterprise).

A standard method for the configuration of the logging system is to use a file based configuration rather than relying on code / a custom environment variable.

I'd propose the following:

  • add an additional command line argument / startup parameter / environment variable to the different products that allows you to specify a logging configuration file, e.g. you can specify the configuration in the following ways
    • rasa run --logging_configuration=my_logging_configration.yml or alternatively
    • RASA_LOGGING_CONFIGURATION=my_logging_configuration.yml rasa run
  • define the logging of the different products using https://docs.python.org/3/library/logging.config.html#dictionary-schema-details, e.g. a configuration would look like this:
    formatters:
      brief:
        format: '%(levelname)-8s: %(name)-15s: %(message)s'
      precise:
        format: '%(asctime)s %(name)-15s %(levelname)-8s %(message)s'
    filters:
      allow_foo:
        name: foo
    handlers:
      console:
        class : logging.StreamHandler
        formatter: brief
        level   : INFO
        stream  : ext://sys.stdout
        filters: [allow_foo]
      file:
        class : logging.handlers.RotatingFileHandler
        formatter: precise
        filename: logconfig.log
        maxBytes: 1024
        backupCount: 3
      debugfile:
        class : logging.FileHandler
        formatter: precise
        filename: logconfig-detail.log
        mode: a
    loggers:
      foo:
        level : ERROR
        handlers: [debugfile]
      spam:
        level : CRITICAL
        handlers: [debugfile]
        propagate: no
      bar.baz:
        level: WARNING
    root:
      level     : DEBUG
      handlers  : [console, file]

Does this solve your configuration needs for logging?

@tmbo
Copy link
Member

tmbo commented Aug 30, 2022

@raoulvm I've moved your changes regarding the exception handling to a separate PR #844

@raoulvm
Copy link
Author

raoulvm commented Sep 1, 2022

@raoulvm I've taken a closer look and ideally I'd like to aim at a unified solution across our different product parts (SDK, Rasa Open Source, Rasa Enterprise).

A standard method for the configuration of the logging system is to use a file based configuration rather than relying on code / a custom environment variable.

I'd propose the following:

  • add an additional command line argument / startup parameter / environment variable to the different products that allows you to specify a logging configuration file, e.g. you can specify the configuration in the following ways

    • rasa run --logging_configuration=my_logging_configration.yml or alternatively
    • RASA_LOGGING_CONFIGURATION=my_logging_configuration.yml rasa run
  • define the logging of the different products using https://docs.python.org/3/library/logging.config.html#dictionary-schema-details, e.g. a configuration would look like this:

    formatters:
      brief:
      ....

Does this solve your configuration needs for logging?

Hi @tmbo this looks great, different though, so I have to look deeper into that. Our current implementation uses a modified logger class, and that is not configurable using the standard dictConfig() so I have to look into re-writing that to move functionality from there to the handler or filter classes. Should be possible though.

Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @raoulvm Checking in if the solution @tmbo released solved your needs and if we can close this PR?

@tmbo
Copy link
Member

tmbo commented Sep 8, 2022

well, we still need to implement the logging part :D (we only shipped the exception handler part of the PR)

@ancalita
Copy link
Member

ancalita commented Sep 8, 2022

Jumped the gun there @tmbo 🙈

@ancalita
Copy link
Member

ancalita commented Nov 3, 2022

@raoulvm I've implemented the changes (that @tmbo mentioned above) in this PR. Could you please take a look and validate if this implementation satisfies your requirements before I merge it imminently? 🙏🏻

@tmbo tmbo closed this Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants