(discuss) Make FastForward an abstract class with default implementation

[clairemcginty]

Prior discussion here: spotify/semantic-metrics#76

This is an extension of the discussion above ^ re: testability in the FastForwardReporter in semantic-metrics repo. Basically, in a production environment the FastForward client would be already running within the container, but in a local unit testing environment this can be pretty cumbersome to set up. I think making FastForward an abstract class (with a default implementation) would be really helpful, because for simple unit tests we could plug in a stubbed implementation that doesn't require a datagram socket running and can easily verify the metrics being sent. I initially suggested making an abstraction layer for it in semantic-metrics, but they suggested the change be more appropriate here.

for example:

public abstract class FastForward {
  public abstract void send(Metric metric) throws IOException;
  public abstract void send(Event event) throws IOException;

  // Default implementation
  public static FastForward setup(String host, int port) throws UnknownHostException, SocketException {
    return new FastForward() {
      private final DatagramSocket socket = new DatagramSocket();
      
      @Override
      public void send(Metric metric) throws IOException {
         // existing implementation here, send through socket
      }

      @Override
      public void event(Event metric) throws IOException {
         // existing implementation here, send through socket
      }
    };
  }
}

and a user could create their own stub implementation for unit testing, like:

public class StubbedFastForwardClient extends FastForward {
  private final List<Metric> sentMetrics;
  private final List<Event> sentEvents;

  public StubbedFastForwardClient(List<Metric> collectedMetrics) {
    this.sentMetrics = new ArrayList<>();
    this.sentEvents = new ArrayList<>();
  }

  @Override
  public void send(Metric metric) throws IOException {
    sentMetrics.add(metric);
  }

  @Override
  public void send(Event event) throws IOException {
    sentEvents.add(metric);
  }

 // Called in unit test
  public void verifySentMetrics(Consumer<List<Metric>> verifyFn) {
    verifyFn.accept(sentMetrics);
  }

  public void verifySentEvents(Consumer<List<Event>> verifyFn) {
    verifyFn.accept(sentEvents);
  }
}

wdyt? I'm happy to make the PR myself it sounds reasonable.

[sming]

Hi Claire, guys,

my 2d (as a spotin00b FYI) is that this should be treated purely as a refactoring. It bears highlighting that refactoring is not making something better or improving its observed behavior. It's solely changing the hidden implementation and explicitly not changing the observable behavior.

Hence if our unit tests have good coverage, and if we observe they pass, we should be pretty confident that the risk of this change exhibiting incorrect behavior is low.

However 🙂 what about the code that links against ffwd-client-java?

1. If that code expects an instantiatiable class, it will fail. Is this expected and planned for? (Sorry, I don't know all prior discussion).
2. Also, if we're changing the "surface" of the lib like this, won't all client code (i.e. coffee that uses ffwd-client-java) need to be rebuilt?
3. Finally (I think!), Wouldn't an interface e.g. interface FastForward be better than an abstract class? Most testing approaches, libraries and so on need an interface to "target" for their runtime machinations. I think that would perhaps enable using Mockito for instance. Thoughts?

Just my 2d.
Cheers, Pete (Backend engineer - Prism)

[clairemcginty]

thanks for chiming in @sming ! Yeah, as I'm mostly familiar with FastForward via the semantic-metrics integration I wasn't sure how much of an impact something like this would have. On the plus side, the constructor for FastForward is private, so I would imagine libraries using it are only instantiating it via the static setup method, which wouldn't change.

Agree that an interface might be better, but would require more code changes, and we would have to move the setup method to an implementing class which would be more of a breaking API change, imo.

Anyway, let me know what you guys decide is best!

[clairemcginty]

btw, put up a simple branch with the proof of concept + a unit test to prove the default implementation still works correctly: https://github.com/spotify/ffwd-client-java/compare/claire/ffwd_abstract_class