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

event logging changes #8076

Closed
wants to merge 3 commits into from
Closed

event logging changes #8076

wants to merge 3 commits into from

Conversation

DiggidyDave
Copy link
Contributor

@DiggidyDave DiggidyDave commented Aug 20, 2019

  • add std logger foundation to event_logger
  • log event for CSV export
  • change "log_this" to more self-documenting "log_requests"
  • add LoggingConfigurator type for user-definable log configuration

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Current incarnation of "event logger" is an "abstract base class", but in practice its usefulness is tightly coupled to the DBEventLogger implementation, which in turn is tightly coupled to a mandatory db migration ('logs' table). The base class itself has a completely untyped interface (args and kwargs), but the DBEventLogger impl relies on very specific (but not codified-via-interface) kwargs to be in the incoming messages. Those kwargs are put there by the log_this decorator which is scattered across the codebase, which means that if you have DBEventLogger configured you can't actually use this AbstractEventLogger outside of the log_this decorator unless you know the specific schema that the DBEventLogger expects to find in the untyped args and kwargs data it receives (which are very request-logging specific fields).

This change is a first step to move logging towards a standard logging based approach with clearer data types with crisp responsibilities surrounding the logging of various event types, and brings the following changes:

  • all incoming events are logged to a configured superset_events logger
  • log_this is renamed to log_requests to more clearly document what it is passing and persisting to the DBEventLogger, and now is logging an event through event logger prior to sending to DBEventLogger
  • LoggingConfigurator is added to allow local installations to configure a custom handler for events
  • query logger is now logging a query event to to event logger prior to calling the configured query logger
  • CSV export is writing a csv_export event to logs

Future work:

  • move log_requests out of AbstactEventLogger base and into its own decorator type which can use an event logger to log events, meaning we can also remove the existing call to log method (leaving only log_event call)
  • turn DBEventLogger into a logging.Handler impl that looks for endpoint_invocation events and does the db persistence that it does today
  • migrate AbstractEventLogger to EventLogger after
  • remove QUERY_LOGGER, that can be handled by registering logging.Handler impls that act upon incoming query event types.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

@mistercrunch @betodealmeida @john-bodley @graceguo-supercat @dpgaspar

- add std logger foundation to event_logger
- log event for CSV export
- change "log_this" to more self-documenting "log_requests"
- add LoggingConfigurator type for user-definable log configuration
@DiggidyDave
Copy link
Contributor Author

I submitted #8085 as a more tactical change to unblock what we need to audit CSV exports, we can revisit the broader changes here a bit later, perhaps more incrementally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants