[metricbeat] migrate mongodb to reporterv2 with error handling #11653
Conversation
fearful-symmetry
added
Metricbeat
| err = errors.New("Unexpected data returned by mongodb") | ||
| logger.Error(err) | ||
| reporter.Error(err) | ||
| m.Logger().Error("Unexpected data returned by mongodb") |
There was a problem hiding this comment.
This is a change in behaviour. Previously we also sent an error event now we only log it. I think we should still send the error event.
| err = errors.Wrap(err, "Mapping of the event data filed") | ||
| logger.Error(err) | ||
| reporter.Error(err) | ||
| m.Logger().Error(errors.Wrap(err, "Mapping of the event data filed")) |
| err = errors.Wrapf(err, "Failed to retrieve stats for db %s", dbName) | ||
| logger.Error(err) | ||
| continue | ||
| m.Logger().Error(errors.Wrapf(err, "Failed to retrieve stats for db %s", dbName)) |
There was a problem hiding this comment.
We lost the continue here and are not reporting the error event anymore.
There was a problem hiding this comment.
Good catch!
| reported := reporter.Event(mb.Event{MetricSetFields: data}) | ||
| if !reported { | ||
| //this means the metricset has closed, no point in trying to log anything further | ||
| return errors.New("metricset has stopped") |
There was a problem hiding this comment.
I think here we should just return nil and not report an error document. Stopping a metricset is not an error.
| // instantiate direct connections to each of the configured Mongo hosts | ||
| mongoSession, err := mongodb.NewDirectSession(m.DialInfo) | ||
| if err != nil { | ||
| logger.Error(err) | ||
| reporter.Error(err) |
There was a problem hiding this comment.
This is still missing here 😄 same in line 76.
There was a problem hiding this comment.
Ah, missed one!
There was a problem hiding this comment.
Wait, do we need to add anything there? My understanding is that we only need to add the error reporting back in where we're not actually returning, i.e, inside a loop
There was a problem hiding this comment.
My understanding is logger.Error(err) is to log the error and reporter.Error(err) is to actually report an event with error msg. So if we only have a return errors.Wrap(...) here, then we are missing the report an event part.
There was a problem hiding this comment.
If we return an error, it's not only logged but also reported: https://github.com/elastic/beats/blob/master/metricbeat/mb/module/wrapper.go#L243
| logger.Debug("error reporting event") | ||
| return | ||
| reported := reporter.Event(mb.Event{MetricSetFields: data}) | ||
| if !reported { |
There was a problem hiding this comment.
We could log on debug level that the metricset was stopped during reporting, but I think that can already be seen in other places, so +1 on this change.
Nit: I preferred the previous 1 line statement ;-)
See #11374
Not much to add to this one.