[metricbeat] migrate rabbitmq to reporterv2 with error return#12228
Conversation
| evt, err := eventMapping(node) | ||
| if err != nil { | ||
| errors = append(errors, err) | ||
| m.Logger().Errorf("error in mapping: %s", err) |
There was a problem hiding this comment.
why do you need to log this here if you are reporting it next?
There was a problem hiding this comment.
Reporting the error will not log it. So the error reported is for the UI / Kibana, the one logged for the operator.
Not sure if we should really log this on the error level.
There was a problem hiding this comment.
Really? I figured if an event was getting thrown out that would be an error.
There was a problem hiding this comment.
In the past we had the problem that when some fields were missing, the schema.Apply returned an error which caused error logs inside. It meant just the event was not complete but it was not dropped. But since then @jsoriano made a few improvements to the schema handling. Please double check on where the errors come from and if these to an actual error happened (not a missing field) +1 on logging it.
There was a problem hiding this comment.
Is there a way to do that? The only time eventMapping will return an error is if schema.Apply fails, and that returns a multi-error structure.
There was a problem hiding this comment.
Looking around the source, the error from schema.Apply() is handled...inconsistently.
grep -rn "err := schema\.Apply" . | wc -l
14
grep -rn "_ := schema\.Apply" . | wc -l
16
There was a problem hiding this comment.
So, we can mark certain values in the schema as required, but then that raises the issue of exactly what is required.
There was a problem hiding this comment.
But I suppose we can set FailOnRequired and at least that'll improve things?
There was a problem hiding this comment.
Just curious how did you pick vhost, host and node in this case? maybe name should be required as well?
|
Okay, so I just put in another PR to use I marked three fields as required: As |
kaiyan-sheng
left a comment
There was a problem hiding this comment.
Thanks for answering my question, looks good to me.
See #11374
This is definitely one of the most complicated migrations yet, and I ended up doing a lot of refactoring to clean up the error handling. I could have done more, but I decided not to go too overboard.
There's a lot of calling
r.Eventinside a loop, which means we need to revert to our pre-ReporterV2 error handling.