Add new interfaces for ReporterV2 with context

[jsoriano]

Alternative implementation of #11451, to provide a context to fetchers
in a more explicit way by receiving a context as parameter of Fetch
or Run.

For that two new interfaces are added with their testing methods.

As example two metricsets are migrated to use the new context, one using
ReporterV2 and another one using PushReporterV2.

[ruflin]

I preferred #11451 as it does not add one more interface and context is hidden in the background. But that is just personal preference.

[jsoriano]

In principle I'd also prefer to avoid adding a new interface. But one of the reasons I don't like of having many interfaces is that every time we change an interface in a module, even if the change is small there we also have to change the tests. It can be also tricky for developers and contributors to know what method to use depending on the interface they are implementing.

#11131 or #11998 can help on reducing the pain points of having many interfaces.

[ruflin]

I like the idea to reduce the paint points around many interfaces but we should still strive to support as few as possible.

[sayden]

If I can give my humble 2 cents 🙂

Generally, I see we don't embrace much of the composition nature in Go. For example the ReportingMetricSetV2, looks like this:

type ReportingMetricSetV2 interface {
	MetricSet
	Fetch(r ReporterV2)
}

When I think it should look more like this:

type ReportingMetricSetV2 interface {
	MetricSet
	FetcherV2
}

type FetcherV2 interface {
	Fetch(r ReporterV2)
}

So when adding error handling we can have this:

type ReportingMetricSetV2Error interface {
	MetricSet
	FetcherV2
	FetcherWithError
}

type FetcherWithError interface {
	FetchWithError(r ReporterV2) error
}

And with context, for example:

type FetcherWithContext interface {
	FetchWithCtx(ctx context.Context, r ReporterV2) error
}

type NiceReporterV2 interface {
	MetricSet
	FetcherWithError
	FetcherWithContext
	FetcherV2
}

And because they don't all have the same method name Fetch they can work around lack of method overloading in Go. Old calledrs to FetcherV2 can still call FetcherWithContext add pass its own context.Background() to maintain compatibility.

BUT 😄 Actually this methodology won't scale well here if we need to keep adding "options" so I think that a better approach would look similar to this:

// Don't try to copy-paste, I did this directly on github xD
type ReporterV3 interface {
	Event(ctx context.Context, event Event, opts ...Option) error // Event reports a single successful event.
	Error(ctx context.Context, err error, opts ...Option) error
}

type FetcherV3 interface {
    FetchWithCtx(ctx context.Context, r ReporterV3) error
}

type MetricsetFetcherV2 interface {
    Metricset
    FetcherV3
}



type Option interface {
	GetOption() interface{}
}



func NewErrorHandlingOption() Option {
	return &ErrorHandlingOption{}
}

type ErrorHandlingOption struct {}

func (c *ErrorHandlingOption) GetOption() interface{} {
    return func(err error){
        fmt.Println(err)
    }
}

Sorry, I couldn't spent time really looking at mb package to give a more clear and concise example but I think that the idea is clear. The idea is to move logic and knowledge to the framework and leave modules with little knowledge of who things works under the hood. Also, the context arrives at the Fetch method and can be propagated during calls and to the Event and Error method

Instead of keep adding interfaces that requires migration of all modules or will have an inconsistent interface use, I think we need a more general approach to this: context and error must always be used or, at least, experience has taught me that 100% of the times you'll regret to not return errors and 90% of not using context when doing network IO.

[jsoriano]

@sayden I am not sure of following, lots of mixed ideas here....

I totally embrace composition 🙂 but I don't think it benefits so much here. For example, what would implement these ReportingMetricSetV2Error and NiceReporterV2 interfaces?

Regarding this ReporterV3 interface, implementers of reporter interfaces are intended to provide "shot and go" communication methods with the framework, they just report events to the framework, and the framework, at a complete different level (and even calling stack), takes care of processing and sending them. This way it is not a responsibility of the fetcher to handle communication problems, they only fetch info and generate events, and they don't care if these events cannot be processed or sent.
Event reporting cannot fail, so methods of the reporter interfaces don't need to return errors, and Fetchers don't need to handle them (remember that the returned bool of Event is not an error, it is more a cordiality notification).
They also don't need any other thing that a context can provide, fetchers don't have (and they shouldn't have) any control on event processing once reported.

The options arguments can be a good thing, and could be added in a backwards compatible way, but I am not sure when we would use them. I'd leave this for the first use case we find. Regarding the error example, I'd leave this discussion for other threads (it was also slightly discussed here).

The idea is to move logic and knowledge to the framework and leave modules with little knowledge of who things works under the hood.

This is exactly the idea of the interfaces we have now, metricsets only need to implement one of the fetcher methods, and the framework takes care of everything else. The recent addition of the error interface also goes in this direction, so metricsets don't have to care on how errors are reported.

In the only point when they have to care about something is in the tests, where they have to use specific methods depending on the interfaces they have implemented, #11131 or #11998 would fix this part by making all metricsets to be tested the same way, no matter what interface they implement.

Instead of keep adding interfaces that requires migration of all modules or will have an inconsistent interface use, I think we need a more general approach to this: context and error must always be used or, at least, experience has taught me that 100% of the times you'll regret to not return errors and 90% of not using context when doing network IO.

Migrating all modules was never in my mind, this is why I originally thought on adding this as a method of the reporter. I'd only migrate the modules that are currently using contexts, and maybe others that would benefit of it, but with very low priority. I don't see such an inconsistency on having a couple of interfaces to choose.

I agree that returning errors and using contexts are good things, but I wouldn't think on forcing all modules to use contexts, several of them would require deeper changes to take advantage of them.

[jsoriano]

@sayden as summary to unblock this a little bit 🙂 what would you prefer?

1. Adding the context as a method of the reporter (Add context to ReporterV2 #11451).
2. Adding a new interface with context as here (Add new interfaces for ReporterV2 with context #11981) and potentially reducing pain points with [Metricbeat] Make integration tests more generic for modules #11131 or Add helper with a common interface for all test reporters #11998.
3. Refactoring current interface and all metricsets so they have context and error, even if the context is ignored by now.
4. Continue as we are now without providing a context to fetchers.

[sayden]

I feel like my order of preference would be

• (3) Because it will help us in the inmediate future probably but it is the most time consuming task of all
• (2) Because it's a more canonical implementation in Go, it helps in the short term and could lead to point 3
• (4) 😄
• (1) Last, because I don't see much the implementation that needs to store the context and it's returned in a method... I don't know but it seems overly complex for the solution it provides and looks like it has many more implications.

Again, that was my 2 cents only, you are good to go with whatever option you consider. All in all you are the one who has spend more time with this now 😉

[jsoriano]

Ok, I will continue with this option, thanks for the feedback.

[jsoriano]

I am going to resurrect this, actually it would be ready for review after deciding that we go with this option.