Middleware capability? For observability and error handling

[alexevanczuk]

Hi there!

I'm interested in adding observability and monitoring into our Danger integration. This will help us use Danger at a larger scale by giving us insights into important qualities of our danger jobs, including:

• Duration to run (to figure out how to tighten feedback loop)
• Number and category (markdown, error, etc.) of messages created (to understand magnitude of feedback on PRs)
• Occurrence of bugs (to respond proactively rather than reactively to bugs)

I've looked through the gem documentation and source and best I can tell there is no current option.

I wanted to share two alternatives, one which is entirely client-side and one which would require upstream changes.

Solution 1: Manual Observability Per Gem

This solution can take many forms, but it's basically saying that the client wraps each check with helpers to observe the gem.

# Dangerfile
DangerObservability.wrap { my_plugin_1.check }
DangerObservability.wrap { my_plugin_2.check }
DangerObservability.wrap { my_plugin_3.lint }

With this approach, it's 100% a client side solution. The client can wrap bugs and decide how to handle them (related: #1352, cc @rymai ), or time the method call, etc. Similarly, the client can drop in a DangerObservability module into each check and have at the bottom of their Dangerfile something that iterates over all Danger::Plugin subclasses and verify they include that instrumentation.

Solution 2:

Another solution is middleware. I found this entry point:

danger/lib/danger/danger_core/dangerfile.rb

Line 29 in fdf91d1

plugin = klass.new(self)

What if we wrapped klass.new(self) with a representation of the middleware stack. So when we call a method on the plugin, it first calls the middleware stack, and then it forwards the method and arguments to the original plugin. The public API may look like:

# config/initializers/danger.rb
class MyMiddleware
  def call(danger_plugin, &block)
    MyObservabilityTool.time('my_metric', { tags: my_tags }, &block)
  end
end

Danger.configure do |config|
  config.middleware = MyMiddleware.new
end

[manicmaniac]

Sounds interesting but I don't know introducing middleware to Danger is better or not 🤔

• Duration to run (to figure out how to tighten feedback loop)
• Number and category (markdown, error, etc.) of messages created (to understand magnitude of feedback on PRs)
• Occurrence of bugs (to respond proactively rather than reactively to bugs)

These goals could be achieved with more Ruby-ish way -- I mean meta-programming like dynamically overriding Dangerfile's methods, setting TracePoint or so -- without changing Danger itself.

[technicalpickles]

I don't know introducing middleware to Danger is better or not

The way I am looking at it is that it would make Danger itself be more extensible, and allow for

Middleware itself is a very common pattern, you can see it in places like rack for example.

These goals could be achieved with more Ruby-ish way -- I mean meta-programming like dynamically overriding Dangerfile's methods, setting TracePoint or so

That is true, but it does involve opening up Danger classes in your own project, and it is a lot of copy/pasting to get similar things working.

As an example, here is a patch I put together to announce when a danger method was running, and what the results for it were as it runs:

require 'benchmark'
# make public so we can inspect later
# do this before allowing patching so adding these methods don't themselves get instrumented
Danger::DangerfileMessagingPlugin.class_eval do
  attr_reader :warnings
  attr_reader :errors
  attr_reader :messages
  attr_reader :markdowns
end

Danger::Plugin.class_eval do
  def self.method_added(name)
    if !private_method_defined?(name)
      patch_announcements_for(name)
    end
  end

  def self.patch_announcements_for(name)
    announcer = Module.new do
      define_method name do |*args, **kwargs|
        $stderr.write "#{self.class}##{name}... "
        result = nil

        warning_count = warnings.size
        error_count = errors.size
        message_count = messages.size
        markdown_count = markdowns.size

        time = Benchmark.realtime do
          result = super(*args, **kwargs)
        end

        if warning_count != warnings.size || error_count != errors.size || message_count != messages.size || markdown_count != markdowns.size
          puts "(#{time.round(2)}s)"
          warnings.each do |violation|
            puts violation.message.indent(2).yellow
          end

          errors.each do |violation|
            puts violation.message.indent(2).red
          end

          messages.each do |violation|
            puts violation.message.indent(2)
          end

          markdowns.each do |violation|
            puts violation.message.indent(2).purple
          end
        else
          warn "#{'pass'.green} (#{time.round(2)}s)"
        end

        result
      end
    end

    prepend announcer
  end
end

From Danger's perspective, is it better to provide an explicit extension point for this type of thing, or expect users to monkey patch like this?