(Discuss) Abstract the FastForward client used in FastForwardReporter

[clairemcginty]

semantic-metrics/ffwd-reporter/src/main/java/com/spotify/metrics/ffwd/FastForwardReporter.java

Lines 156 to 159 in 3890ddd

	public Builder fastForward(FastForward client) {
	this.client = client;
	return this;
	}

The current setup for FastForwardReporter requires a user to pass in a real FastForward client, which is not easily unit testable -- 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.

It would be simpler if we had some kind of interface or abstract class to allow the user to sub in different implementations of the FastForward client -- the only method it would have to implement is public void send(Metric metric) { }.

Quick code sketch of what I'm thinking:

public abstract class FastForwardClient {
  public abstract void send(Metric metric) throws IOException;

  // Default implementation
  public static FastForwardClient create(String host, int port) throws UnknownHostException, SocketException {
    return new FastForwardClient() {
      private final FastForward underlying = FastForward.setup(host, port);
      
      @Override
      public void send(Metric metric) throws IOException {
        underlying.send(metric);
      }
    };
  }
}

Then the FastForwardReporter's builder would look like:

public Builder fastForward(FastForwardClient client) {
  this.client = client;
  return this;
}

public FastForwardReporter build() throws IOException {
  final FastForwardClient client = this.client != null ? this.client : FastForwardClient.create(host, port);
  ...
}

And in a unit-testing context we could easily sub in something like this that's easy to validate:

public class StubbedFastForwardClient extends FastForwardClient {
  private final List<Metric> collectedMetrics;

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

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

  public List<Metric> getCollectedMetrics() {
    return collectedMetrics;
  }
}

lmk what you think! I'm happy to make the PR myself if this seems reasonable.

(Not sure if this issue belongs more in ffwd-client-java... it would probably be better to fix it there, but it's also a bigger/more impactful change to make.)

[malish8632]

It looks reasonable. I guess mocking won't help if you want exact list of sent metrics.

[clairemcginty]

cool, thanks! yeah, it depends on what you're trying to unit test exactly, in some cases using the real FastForward client would still be appropriate.

How do you feel about me making this change in semantic-metrics as opposed to making a larger change in the ffwd-client-java lib (turning FastForward into an abstract class with a default implementation)? I can do either one but I'm less confident about making changes in the ffwd-client-java lib.

[ao2017]

Hello @clairemcginty
Agreed it is not easy to test the send method in FastForward. However FastForward is not a final class you can essentially do what you want to do without making it abstract.
public class StubbedFastForwardClient extends FastForward {
// override send here.

}

[clairemcginty]

hey @ao2017 ! unfortunately as it is now we can't do that because there's no non-private constructor available in FastForward

[malish8632]

hey @clairemcginty, could you elaborate on why you think changing this in ffwd-client-java bigger / more impactful? Do you mean more places to update/change if you want to use this new feature/ability?
It feels the change would be most appropriate in ffwd-client-java.

[clairemcginty]

@malish8632 it's mostly because I'm less familiar with the FastForward lib itself and less sure if it's a breaking change in any way. I'll file a ticket there and link this one 👍

[ao2017]

hey @ao2017 ! unfortunately as it is now we can't do that because there's no non-private constructor available in FastForward

@clairemcginty good point. My comment was based on StubbedFastForwardClient above which implies the existence of a default constructor. Anyway, about just adding a protected constructor ?

[ao2017]

@clairemcginty I added a protected default constructor to FastForward to address this issue spotify/ffwd-client-java#13.