Add new MetricSet interfaces for Module Developers #3908

andrewkroh · 2017-04-04T16:56:11Z

This PR adds new interfaces for MetricSet developers. These interfaces provided additional flexibility for MetricSet implementations. The new interfaces are:

Closer - With Metricbeat gaining the ability to dynamically load and unload modules there became a need to properly release resources opened by a MetricSet. If a MetricSet implements the Closer interface then the Close() error method will be invoked when the MetricSet is unloaded.
ReportingFetcher - Some MetricSets needed to report multiple errors, but the existing Fetch methods only allowed a single error to be returned. ReportingFetcher passes a callback interface to the MetricSet that allows it to report any combination of events, errors, or errors with metadata.
PushMetricSet - This is for push event sources. All the current MetricSet implementations are driven by periodic Fetch callbacks triggered by timer. A PushMetricSet does not receive periodic callbacks, but instead has a Run method. Implementations can forward events as they are received from the underlying source.

ruflin

Not all usage of the interfaces is 100% clear to me yet, but I assume they will get answered as soon as we have the first implementation.

For the ReportingFetcher: Wouldn't it be enough to have Fetch() (common.MapStr, []error) as an interface? How will the additional meta data and errors used? I'm aware you are no big fan of having a "special" Error object but perhaps something like this could be used here?

ruflin · 2017-04-05T06:52:12Z

metricbeat/mb/mb.go

@@ -84,6 +90,42 @@ type EventsFetcher interface {
 	Fetch() ([]common.MapStr, error)
 }

+// Reporter is used by a MetricSet to report events, errors, or errors with
+// metadata. The methods return false iff publishing failed because the


iff is shorthand for "if and only if". I thought it was more common, maybe only common in math. I'll expand it to "if and only if".

lol, makes sense now. Did not make the connection when reading it and just thought: typo :-)

ruflin · 2017-04-05T06:55:07Z

metricbeat/mb/module/event.go

@@ -59,7 +59,9 @@ func (b EventBuilder) Build() (common.MapStr, error) {
 	metricsetData := common.MapStr{
 		"module": b.ModuleName,
 		"name":   b.MetricSetName,
-		"rtt":    b.FetchDuration.Nanoseconds() / int64(time.Microsecond),
+	}
+	if b.FetchDuration != 0 {


Nice, that means we also remove this value in the system module case 👍

ruflin · 2017-04-05T06:57:43Z

metricbeat/mb/module/wrapper.go

 	default:
-		return fmt.Errorf("MetricSet '%s/%s' does not implement a Fetcher "+
-			"interface", msw.Module().Name(), msw.Name())
+		panic(fmt.Sprintf("unexpected fetcher type for %v", msw))


Why do you panic here instead of returning an error?

It's an unreachable code path and can never happen due to an earlier check in the caller. So if it does happen I want it to blow up loudly and early so we can catch it in development/unit tests.

ruflin · 2017-04-05T07:04:47Z

metricbeat/mb/mb.go

+type Reporter interface {
+	Event(event common.MapStr) bool
+	ErrorWith(err error, meta common.MapStr) bool
+	Error(error) bool


Do we need both interfaces or could we just have Error(err error, meta common.MapStr) and in case the meta does not exist it is set to nil? Thinking if we could reduce the number of interface methods.

I think having separate methods for each of the cases (will) improve the readability of MetricSets that implement ReportingFetcher.

Maybe it's not clear that MetricSets don't implement this interface, they only use it. So reducing the number of methods doesn't change the amount of methods a MetricSet needs to implement (if that was your concern).

andrewkroh · 2017-04-05T13:41:23Z

Not all usage of the interfaces is 100% clear to me yet, but I assume they will get answered as soon as we have the first implementation.

I would like to make it clear from the godocs. What would improve your understanding? More docs on the MetricSet types? An example for for each MetricSet type (ReportingFetcher, PushMetricSet) like we have for EventFetcher?

andrewkroh · 2017-04-05T13:48:48Z

For the ReportingFetcher: Wouldn't it be enough to have Fetch() (common.MapStr, []error) as an interface? How will the additional meta data and errors used?

No, that interface wouldn't be sufficient because you cannot report multiple events. Nor can you send data with an error.

How will the additional meta data and errors used?

Maybe only a portion of the metrics were able to be collected due to an error. You can report the metrics you have available and also report the problem that occurred. Perhaps @urso had a different vision for ErrorWith(err, meta) than me?

ruflin · 2017-04-05T19:23:09Z

metricbeat/mb/module/wrapper.go

+	timestamp := r.start
+	elapsed := time.Duration(0)
+
+	if !timestamp.IsZero() {


Minor comment / thought: In Fetch() []common.MapStr all events have the exact same time which elapsed. In this new interface the first event will have a lower rtt then the last event. Not sure yet if this is a good or bad thing.

This PR adds new interfaces for MetricSet developers. These interfaces provided additional flexibility for MetricSet implementations. The new interfaces are: - `Closer` - With Metricbeat gaining the ability to dynamically load and unload modules there became a need to properly release resources opened by a MetricSet. If a MetricSet implements the `Closer` interface then the `Close() error` method will be invoked when the MetricSet is unloaded. - `ReportingFetcher` - Some MetricSets needed to report multiple errors, but the existing `Fetch` methods only allowed a single error to be returned. `ReportingFetcher` passes a callback interface to the MetricSet that allows it to report any combination of events, errors, or errors with metadata. - `PushMetricSet` - This is for push event sources. All the current MetricSet implementations are driven by periodic `Fetch` callbacks triggered by timer. A `PushMetricSet` does not receive periodic callbacks, but instead has a `Run` method. Implementations can forward events as they are received from the underlying source.

- Rename ReportingFetcher to ReportingMetricSet. - Update example in godocs to implement ReportingMetricSet. - Change iff in godocs to “if and only if”.

ruflin · 2017-04-07T07:06:21Z

metricbeat/mb/example_metricset_test.go

+	if err != nil {
+		// Report an error if it occurs.
+		report.Error(err)
+		return


That pattern we will probably see quite often. If Error would not return anything, we could use return report.Error(err) probably.

ruflin

@andrewkroh Ok for me to move forward on this. We definitively need some of these new capabilities for upcoming Modules. I'm still not 100% sure which interfaces we should keep in the end but time will show.

ruflin · 2017-04-07T07:10:13Z

metricbeat/mb/mb.go

 type EventFetcher interface {
 	MetricSet
 	Fetch() (common.MapStr, error)
 }

 // EventsFetcher is a MetricSet that returns a multiple events when collecting
-// data.
+// data. Use ReportingMetricSet for new MetricSet implementations.


In case we decided to keep both, we need to rename it later to something like EventsMetricSet to be consistent. I still like the old Fetch interfaces so not sure yet if Reporter will replace it or just be an other option.

andrewkroh added enhancement Metricbeat Metricbeat review v6.0.0-alpha1 labels Apr 4, 2017

andrewkroh force-pushed the feature/mb/event-reporter branch from 08c75b0 to 74cc2f5 Compare April 4, 2017 16:57

ruflin reviewed Apr 5, 2017

View reviewed changes

andrewkroh requested a review from urso April 5, 2017 11:38

urso approved these changes Apr 5, 2017

View reviewed changes

ruflin reviewed Apr 5, 2017

View reviewed changes

andrewkroh added 2 commits April 5, 2017 20:36

Address review comments

8b8a86d

- Rename ReportingFetcher to ReportingMetricSet. - Update example in godocs to implement ReportingMetricSet. - Change iff in godocs to “if and only if”.

andrewkroh force-pushed the feature/mb/event-reporter branch from 74cc2f5 to 8b8a86d Compare April 6, 2017 02:42

ruflin reviewed Apr 7, 2017

View reviewed changes

ruflin approved these changes Apr 7, 2017

View reviewed changes

ruflin merged commit f3079d9 into elastic:master Apr 7, 2017

andrewkroh deleted the feature/mb/event-reporter branch July 5, 2017 22:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new MetricSet interfaces for Module Developers #3908

Add new MetricSet interfaces for Module Developers #3908

andrewkroh commented Apr 4, 2017

ruflin left a comment

ruflin Apr 5, 2017

andrewkroh Apr 5, 2017

ruflin Apr 5, 2017

ruflin Apr 5, 2017

ruflin Apr 5, 2017

andrewkroh Apr 5, 2017

ruflin Apr 5, 2017

andrewkroh Apr 5, 2017

andrewkroh commented Apr 5, 2017

andrewkroh commented Apr 5, 2017

ruflin Apr 5, 2017

ruflin Apr 7, 2017

ruflin left a comment

ruflin Apr 7, 2017

Add new MetricSet interfaces for Module Developers #3908

Add new MetricSet interfaces for Module Developers #3908

Conversation

andrewkroh commented Apr 4, 2017

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh commented Apr 5, 2017

andrewkroh commented Apr 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment