[Metricbeat] documenting/standardizing the return of r.Event

[fearful-symmetry]

This came up during a discussion with @ruflin and he asked me to file an issue.

So, there's some inconsistency about how we handle the bool that gets returned from reporter.Event(). This will only return false if the metricset has closed.

The vast majority of metricsets ignore this, however a few check the value and log an error:

./ceph/cluster_disk/cluster_disk.go:77: if reported := reporter.Event(mb.Event{MetricSetFields: event}); !reported {
./mongodb/dbstats/dbstats.go:89:                reported := reporter.Event(mb.Event{MetricSetFields: data})
./kubernetes/state_container/state_container.go:144:            if reported := reporter.Event(mb.Event{
./kubernetes/state_replicaset/state_replicaset.go:116:          if reported := reporter.Event(mb.Event{
./kubernetes/state_deployment/state_deployment.go:117:          if reported := reporter.Event(mb.Event{
./kubernetes/state_statefulset/state_statefulset.go:115:                if reported := reporter.Event(mb.Event{

How should we standardize and document this? A blurb in the beats developer docs would be helpful, at least.

[jsoriano]

@fearful-symmetry look at this thread for a related discussion: #9907 (comment)

Some ideas from there:

• This returned value should be used to stop doing anymore work in fetchers that send multiple events
• This can be completely ignored in fetchers that only send an event per fetch
• If anything is logged, it should be done at the debug level, as this is something expected to happen

[ruflin]

++ on the above. I'm even wondering if we should log something on the debug level as I'm pretty sure something else logs already on the debug level that sending was interrupted / metricset was stopped.

Now we need to document the above somewhere in the Metricbeat modules dev docs. @fearful-symmetry Want to take this on?

[fearful-symmetry]

@ruflin sure!

[fearful-symmetry]

This also sorta ties into the "best practices" I mention in #11102

[fearful-symmetry]

This should be covered by #11745

[ruflin]

@fearful-symmetry Should we close the issue?

[fearful-symmetry]

@ruflin unless you think there's additional documentation needed not covered by #11745, we should be good to close it.

I also have a lurking suspicion that there's a few metricsets not using the bool value inside a loop, but that can be another case/PR if I run across them.

[fearful-symmetry]

Okay, so I did gather a list of metricsets that call Event() in a loop but don't check the return to see if the metricset is closed. These probably aren't too urgent, but maybe worth tracking

• all of postgres
• docker/image
• all of vsphere
• most of ceph
• jolokia/jmx
• mongodb/collstats
• most of system
• consul/agent
• kafka/partition
• kvm/dommemstat
• aerospike/namespace
• prometheus/collector
• windows/perfmon
• windows/service
• couchbase/bucket
• couchbase/node
• aws (Check return from report.Event and migrate to use ReportingMetricSetV2Error #11775)

[ruflin]

@fearful-symmetry I would say let's close it but have the list you collected above in a separate issue, otherwise we keep extending the scope of this issue which was only around docs.