Check return from report.Event and migrate to use ReportingMetricSetV2Error #11775
Conversation
There was a problem hiding this comment.
this is looking good but I would want a review from @fearful-symmetry or @ruflin who have been involved in the refactor
|
This looks good, as the added logging/reporting steps only happen inside a loop with |
| report.Event(event) | ||
|
|
||
| if reported := report.Event(event); !reported { | ||
| return errors.Wrap(err, "Error trying to emit event") |
There was a problem hiding this comment.
This is not an error, this is expected to happen. It only means that the module is being stopped. We interrupt the loop to avoid doing more requests or processing that are not going to be used.
Just log something at the debug level (or nothing at all), and return nil.
There was a problem hiding this comment.
Thanks @jsoriano for the explanation!
|
|
||
| moduleConfig := aws.Config{} | ||
| if err := base.Module().UnpackConfig(&moduleConfig); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if moduleConfig.Period == "" { |
There was a problem hiding this comment.
How is this change related?
There was a problem hiding this comment.
Not related. Sorry forgot to update on the PR . This check is not needed here because it's already done in aws.NewMetricSet(base) in line 60.
|
|
||
| moduleConfig := aws.Config{} | ||
| if err := base.Module().UnpackConfig(&moduleConfig); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if moduleConfig.Period == "" { |
Instead of ignoring the return from report.Error, we should actually check the return to see if report event succeed or not. Please see #11666 for more details on this. Thanks @fearful-symmetry to bring this up.
This PR also removes
moduleConfig.Periodcheck in s3 metricset code because it's already done inNewMetricSetin aws.go.