Add handle failed events option to parse json processors #4844
Conversation
graytaylor0
requested review from
chenqi0805,
engechas,
dinujoh,
kkondaka,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| import java.util.Map; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public enum HandleFailedEventsOption { |
There was a problem hiding this comment.
Can you replace the existing option in drop processor with this? The drop and drop_silently options from that processor seems a decent option to provide elsewhere.
There was a problem hiding this comment.
I originally wanted to do this, but if we don't support those two options in the parse processors, then it's a bit weird to have there isn't it? Although I could just add a validation that does not allow drop or drop_silently for these processors
There was a problem hiding this comment.
Yes, I think a validation would be appropriate.
| @@ -29,6 +31,7 @@ | |||
|
|
|||
| public abstract class AbstractParseProcessor extends AbstractProcessor<Record<Event>, Record<Event>> { | |||
| private static final Logger LOG = LoggerFactory.getLogger(AbstractParseProcessor.class); | |||
| private static final String PROCESSING_ERRORS = "processingErrors"; | |||
There was a problem hiding this comment.
I think we should differentiate our metrics between two situations. The first are expected errors. These are bad input that we know will happen. The second are unexpected errors. These are probably bugs.
I think we could have a metric for the first - parseErrors. If we want a metric for the second, this could be named something like processingFailures. Maybe we'd use this in other processors as well as time goes on.
| @@ -100,7 +110,10 @@ public Collection<Record<Event>> doExecute(final Collection<Record<Event>> recor | |||
| event.delete(this.source); | |||
| } | |||
| } catch (Exception e) { | |||
| LOG.error(EVENT, "An exception occurred while using the {} processor on Event [{}]", getProcessorName(), record.getData(), e); | |||
| processingErrorsCounter.increment(); | |||
There was a problem hiding this comment.
I think this is a bit broad. This metric could occur if we have a problem that is not necessarily a parsing error (e.g. event.delete().
Related to my comment above, this might work better if we split the exceptions into two kinds.
graytaylor0
force-pushed
the
JsonHandleFailures
branch
3 times, most recently
from
130ae42
to
a7a4db8
Compare
Signed-off-by: Taylor Gray <tylgry@amazon.com>
graytaylor0
force-pushed
the
JsonHandleFailures
branch
from
a7a4db8
to
5ad4e2a
Compare
| "which will send the Event downstream to the next processor without logging the error. " + | ||
| "Default is 'skip'.") | ||
| @NotNull | ||
| private HandleFailedEventsOption handleFailedEventsOption = HandleFailedEventsOption.SKIP; |
There was a problem hiding this comment.
This means, default behavior is not changed, right? ie, the failures are logged and skipped. This also, means customers will see this issue, unless they explicitly change this. Is this what is agreed upon? Should we set it to SKIP_SILENTLY in OSI?
There was a problem hiding this comment.
Yes that's right default is to log and skip still. OSI default decision will be made separately
…project#4844) Signed-off-by: Taylor Gray <tylgry@amazon.com> Signed-off-by: Krishna Kondaka <krishkdk@dev-dsk-krishkdk-2c-bd29c437.us-west-2.amazon.com>
Description
The parse processors (json, ion, xml) now have support for a
handle_failed_eventsconfiguration that can either be set toskiporskip_silently. In both cases, the Event is sent downstream, but forskip_silently, the processing error will not be logged.Also adds two new metrics
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.