New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new MetricSet interfaces for Module Developers #3908

Merged
merged 2 commits into from Apr 7, 2017

Conversation

Projects
None yet
3 participants
@andrewkroh
Member

andrewkroh commented Apr 4, 2017

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?

@@ -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

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

s/iff/if

This comment has been minimized.

@andrewkroh

andrewkroh Apr 5, 2017

Member

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".

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

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

@@ -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 {

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

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

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))

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

Why do you panic here instead of returning an error?

This comment has been minimized.

@andrewkroh

andrewkroh Apr 5, 2017

Member

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.

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

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

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.

This comment has been minimized.

@andrewkroh

andrewkroh Apr 5, 2017

Member

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 andrewkroh requested a review from urso Apr 5, 2017

@andrewkroh

This comment has been minimized.

Member

andrewkroh commented Apr 5, 2017

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

This comment has been minimized.

Member

andrewkroh commented Apr 5, 2017

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?

@urso

urso approved these changes Apr 5, 2017

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

This comment has been minimized.

@ruflin

ruflin Apr 5, 2017

Collaborator

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.

andrewkroh added some commits Apr 3, 2017

Add new MetricSet interfaces for Module Developers
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.
Address review comments
- Rename ReportingFetcher to ReportingMetricSet.
- Update example in godocs to implement ReportingMetricSet.
- Change iff in godocs to “if and only if”.
if err != nil {
// Report an error if it occurs.
report.Error(err)
return

This comment has been minimized.

@ruflin

ruflin Apr 7, 2017

Collaborator

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

@ruflin

ruflin approved these changes Apr 7, 2017

@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.

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.

This comment has been minimized.

@ruflin

ruflin Apr 7, 2017

Collaborator

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.

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

5 checks passed

CLA Commit author has signed the CLA
Details
codecov/patch 78.35% of diff hit (target 65.54%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
default Build finished.
Details

@andrewkroh andrewkroh deleted the andrewkroh:feature/mb/event-reporter branch Jul 5, 2017

athom added a commit to athom/beats that referenced this pull request Jan 25, 2018

Add new MetricSet interfaces for Module Developers (elastic#3908)
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment