search

hudl / luigi-monitor

Send summary messages of your Luigi jobs to Slack
46 stars 16 forks source link

Discussion: Output configurability #22

Open dlstadther opened 6 years ago

dlstadther commented 6 years ago

Proposal:

Refactor configurability of luigi-monitor, particularly the output format/style/quantity.

Background

Currently, there exists 2 options in the monitor context manager which control output of luigi-monitor - events, and max_print.

However, in PR #20 messages now include a list of successfully ran tasks which is not configurable. This is especially problematic for very large WrapperTasks which execute complex pipelines of hundreds of Tasks. Slack channels get very muddy and unreadable.

Conversation Questions

1) What are thoughts on setting a default behavior which is very very minimal and allowing configuration options to increase the number of failure outputs, success outputs, emoji use, etc? 2) As many configuration options become available, we may want to refactor the structure of code such that each method doesn't have to have tons of args/kwargs - possibly some sort of configuration class (imagine something like Luigi.Config)? 3) Would such a code overhaul necessitate a milestone version bump (i.e. 2.0.0)?

atharvai commented 6 years ago

First of all, thanks for putting forward this proposal

My thoughts:

  1. There are two points here. printing out successful tasks and highly configurable options.
    1. Success tasks: Currently we are able to exclude SUCCESS tasks from Slack output using events kwarg. The default is events=['FAILURE', 'DEPENDENCY_MISSING', 'SUCCESS'] but you can change it to events=['FAILURE', 'DEPENDENCY_MISSING'] to not output successful tasks. Having said that I would like to see default behaviour changed to exclude SUCCESS from events.
    2. Configuration: I think it is a good idea to make each of pretty printing features configurable. One thing for sure that needs to be configurable is emojis. Emojis are great visually but if luigi-monitor is to include other targets (Jira, MS Teams, event collectors (SignalFX, DataDog,etc), etc) excluding emojies might make sense. I'm not committing to adding these destinations but there's no reason luigi-monitor should be limited to Slack.
  2. I like the idea. Following on from above, a YAML based config file like with luigi can be very useful. I wonder if Luigi.configuration could simply be reused or even parts of it. ref: https://luigi.readthedocs.io/en/stable/configuration.html#configuration-classes
  3. I imagine migrating configs would be a major version bump but refactoring and adding configs for emojis could be minor version bump.

If we were to do all 3 of the proposed, I imagine this to be staged release. First would be to refactor the code and keep the same functionality so that configuring each methods for existing options is easier. Then we could refactor to introduce the yaml based config options for existing and mark existing api as deprecated. so these two could be packages as v2.0.0. The final stage would be to add new config options for various outputs, emoji use, etc. This could be minor version bump since there would be backwards compatibility with v2.0.0.

dlstadther commented 6 years ago

Apologies for my delay in response here. I will be spending small bits of time over the coming weeks attempting to refactor luigi-monitor such to maintain backwards compatibility as well as providing additional flexibility towards the goals mentioned above. As I have meaningfully complete refactors, I will contribute them here.

I will leave it up to you, @atharvai , as to whether to keep this issue open or not.

Thanks,