use interface instead of *stats.Engine in httpstats

[mihai-chiorean]

the only method used is ReportAt. This way the consumer (httpstats) is asking for a specific behavior (ReportAt).

Benefits:

• the behavior required is more clear and readable
• mocks can be used to test that the reporting is actually called where expected

[f2prateek]

for testing, have you considered using a stats.Engine with a test handler (https://github.com/segmentio/stats/blob/master/statstest/handler.go)?

[abraithwaite]

I'm with @f2prateek here. I don't think this is necessary and would set a precedent for supporting something like this for other methods on the Engine, which I don't think we should do.

[mihai-chiorean]

for testing, have you considered using a stats.Engine with a test handler (/statstest/handler.go@master)?

I'm using the transport wrapper for a client. The right way to use it would be to instantiate an engine and give it a statstest.Handler, right?
My bad for missing this, I think I can definitely use that for testing.
That being said, I still think using an interface like this on the "consumer" side is an improved design - unless you don't want to reduce coupling between httpstats and stats.

I'm with @f2prateek here. I don't think this is necessary and would set a precedent for supporting something like this for other methods on the Engine, which I don't think we should do.

can you elaborate a little more why you think this is a bad idea?

It does open the door to potentially other implementations to be used with this (sub)package and it does remove some of the coupling between httpstats and stats. However it also informs what exactly the the transport requires vs a whole struct/specific implementation.

[f2prateek]

One of the concerns is that stats.Engine has many functions (https://godoc.org/github.com/segmentio/stats#Engine), and it would be impractical to make new types for each of these functions (and their permutations). I'm not against decoupling, but to me the overhead here feels too high to be worth the tradeoff.

If it helps, you can think of the setup similar to http.Client (equivalent to stats.Engine) and http.RoundTripper (equivalent to stats.Handler).

[mihai-chiorean]

One of the concerns is that stats.Engine has many functions (godoc.org/github.com/segmentio/stats#Engine), and it would be impractical to make new types for each of these functions (and their permutations). I'm not against decoupling, but to me the overhead here feels too high to be worth the tradeoff.

If it helps, you can think of the setup similar to http.Client (equivalent to stats.Engine) and http.RoundTripper (equivalent to stats.Handler).

Got it. I look at interfaces in a different way than types. Only the consumer of engine - or a subset Engine's API - would have an interface, essentially exposing just the subset to the consumer.

Thanks for the feedback!

[abraithwaite]

It does open the door to potentially other implementations to be used with this (sub)package and it does remove some of the coupling between httpstats and stats.

This is one reason yeah, you're opening the door for other implementations to be injected into this (which we'd have to support). As a general rule, library authors should avoid exporting unnecessary interfaces.

However it also informs what exactly the the transport requires vs a whole struct/specific implementation.

While it's true that it defines the exact signature, it doesn't define the exact behavior (and it will never have the exact same behavior, otherwise it'd be the same implementation). As a contrived example, imagine a bug report we get down the line from someone who'd implemented their own Reporter and forgotten about it but is wondering why they're not getting metrics anymore.

I'll defer to @achille-roussel on whether or not he wants to merge this, but hopefully I was able to answer some of your questions. If you want it explained by someone a bit more articulate than myself, check out this blog post by jbd: https://rakyll.org/interface-pollution/

In summary, if the only reason for adding a new interface is for testing than it might not be appropriate to add that new interface as an exported type.

[mihai-chiorean]

It does open the door to potentially other implementations to be used with this (sub)package and it does remove some of the coupling between httpstats and stats.

This is one reason yeah, you're opening the door for other implementations to be injected into this (which we'd have to support). As a general rule, library authors should avoid exporting unnecessary interfaces.

However it also informs what exactly the the transport requires vs a whole struct/specific implementation.

While it's true that it defines the exact signature, it doesn't define the exact behavior (and it will never have the exact same behavior, otherwise it'd be the same implementation). As a contrived example, imagine a bug report we get down the line from someone who'd implemented their own Reporter and forgotten about it but is wondering why they're not getting metrics anymore.

I'll defer to @achille-roussel on whether or not he wants to merge this, but hopefully I was able to answer some of your questions. If you want it explained by someone a bit more articulate than myself, check out this blog post by jbd: rakyll.org/interface-pollution

In summary, if the only reason for adding a new interface is for testing than it might not be appropriate to add that new interface as an exported type.

Thanks @abraithwaite! I appreciate the thoughtful response!