Add context to ReporterV2

[jsoriano]

Add a context.Context based on the done channel metricset
wrapper receives. This way all Fetch() methods will have a context
that can be used in their clients and requests.

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

[ruflin]

@andrewkroh Would be great to get your opinion on this change.

[andrewkroh]

I like the change. I think I originally didn't go for context.Context because it was a pretty new API at the time.

With this change it seems that there is no longer a need to have a separate Reporter and PushReporter. So I think we can deprecate the PushReporter interfaces. Existing push reporters can migrate to use the context's Done() method.

Another thought I had (but no investigation) is that Metricbeat metricset's have a configurable timeout option IIRC. I don't think this option is used much (or implemented properly in most metricsets), but when it is used we could pass a context that has been wrapped WithTimeout.

[sayden]

I'm afraid that I believe that we shouldn't add context at all 😅 There are two main rules when implementing context:

• context.Context must be propagated from the initial call so func ReportingFetchV2(metricSet mb.ReportingMetricSetV2) ([]mb.Event, []error) must be func ReportingFetchV2(ctx context.Context, metricSet mb.ReportingMetricSetV2) ([]mb.Event, []error). Propagation is key when using context.
• context.Context must not be stored anyhow. It's a continuation of the previous, if you follow the rule of propagation, you won't store it at all.

Finally, I don't see a real benefit of using context here. I mean that it looks like it's a "we can use Context here, as many other approaches" instead of a "it's a clear use case of Context". Also, I feel that we are replacing a piece with a explicit implementation which was already working. Explicit code is idiomatic in Go :)

A not so old post about this: https://dave.cheney.net/2017/08/20/context-isnt-for-cancellation

[jsoriano]

I'm afraid that I believe that we shouldn't add context at all 😅 There are two main rules when implementing context:
* context.Context must be propagated from the initial call so func ReportingFetchV2(metricSet mb.ReportingMetricSetV2) ([]mb.Event, []error) must be func ReportingFetchV2(ctx context.Context, metricSet mb.ReportingMetricSetV2) ([]mb.Event, []error). Propagation is key when using context.

* `context.Context` must not be stored anyhow. It's a continuation of the previous, if you follow the rule of propagation, you won't store it at all.

In general I agree with that, but I'd like to avoid adding yet another interface (two of them if adding the one returning an error, plus its testing related code). We'd have to evaluate if it worths to add more interfaces for this.

Also this is a pattern also used in the standard library, for example HTTP handlers receive a Request that has a Context().

The context is only being stored where it is created, in the wrapper. We can continue with future refactorizations to replace Start(done <-chan struct{}) with Start(ctx context.Context) so we have a context that can be propagated from there.

Finally, I don't see a real benefit of using context here. I mean that it looks like it's a "we can use Context here, as many other approaches" instead of a "it's a clear use case of Context". Also, I feel that we are replacing a piece with a explicit implementation which was already working. Explicit code is idiomatic in Go :)

Well, most of our modules make requests to other services and many client libraries accept contexts.
The main benefit is that we can start using a context that makes sense in the current module lifecycle instead of using Background(), TODO() or avoiding APIs with contexts.

It is not using it "as many other approaches", it is using it as the approach that afaik is more widely used in the standard and third party libraries (even if we can discuss if it is the best approach).

A not so old post about this: https://dave.cheney.net/2017/08/20/context-isnt-for-cancellation

Nice read, but I think that start using something like tomb would require a huge effort in the case of Metricbeat, while getting a context from a done channel we already have is quite straightforward. Also many libraries we use expect contexts, if we use other things we still need to convert them.

[ruflin]

When the first version of Beats was written, Context did not exist yet and we started with the done channel. Would we build it again I would assume Context would play quite a role in it. Long term I think more and more parts should be switched over to context but we need to go baby steps here (like in this PR). At least that is my thinking.

[jsoriano]

Thanks @andrewkroh and @sayden for your comments.

I totally agree about deprecating the PushReporter interface if we do this, but I'd leave it for a future change. We'd still need a new runner interface for that.

Regarding the timeout option, afaik it is only used in modules using the HTTP helper. With this context we could make it a more general setting, and we could also refactor the helpers to do requests with contexts.

[ruflin]

@jsoriano Could you rebase this on master? Quite a few of the failing tests should have been fixed by now.

[ruflin]

Can you add the PR number?

[exekias]

+1 to using context, we have been bitten by wild requests in the past. From what I understand, you went with passing context as part of reporter (instead of arg to Fetch/Run) because:

• We already migrated all metricsets to V2 -> we don't care about V1 anymore
• You don't want to change V2 API by adding context parameter there

In general I'm good with the compromise, although having it explicit as a parameter would avoid some user mistakes, as you need to know reporter offers a Context() method

[jsoriano]

@exekias @sayden I have opened a draft with the changes needed to do it with an explicit parameter, let me know what you think #11981

[exekias]

I don't have an strong preference, the new one is more explicit. What do others think? @sayden @andrewkroh

[sayden]

I left some comments 🙂

[jsoriano]

I will continue with the more explicit approach in #11981, thanks all for the feedback.