Write json processor #4514
Write json processor #4514
Conversation
Signed-off-by: Krishna Kondaka <[email protected]>
Signed-off-by: Krishna Kondaka <[email protected]>
kkondaka
requested review from
chenqi0805,
engechas,
graytaylor0,
dinujoh,
asifsmohammed,
KarstenSchnitter,
dlvenable and
oeyh
as code owners
| try { | ||
| event.put(target, objectMapper.writeValueAsString(value)); | ||
| } catch (Exception e) { | ||
| LOG.error("Failed to convert source to json string"); |
There was a problem hiding this comment.
Can we log the exception message here? Could also have a metric for errors and tags as well, although that is not required for this PR
There was a problem hiding this comment.
how should customer handle when the record is not converted into json string ?
There was a problem hiding this comment.
@dinujoh In pipeline, error cases are logged or metrics are incremented. There is not much we can do about it.
Signed-off-by: Krishna Kondaka <[email protected]>
| try { | ||
| event.put(target, objectMapper.writeValueAsString(value)); | ||
| } catch (Exception e) { | ||
| LOG.error("Failed to convert source to json string", e); |
There was a problem hiding this comment.
Does the exception contain user data ?
There was a problem hiding this comment.
We can fix this as followup to mask customer data.
There was a problem hiding this comment.
Here is an example of marker
Signed-off-by: Krishna Kondaka <[email protected]>
...rc/main/java/org/opensearch/dataprepper/plugins/processor/write_json/WriteJsonProcessor.java
Outdated
Show resolved
Hide resolved
| target = writeJsonProcessorConfig.getTarget(); | ||
| if (target == null) { | ||
| target = source; | ||
| } |
There was a problem hiding this comment.
Using Java 9 Objects class would allow to make target final:
| target = writeJsonProcessorConfig.getTarget(); | |
| if (target == null) { | |
| target = source; | |
| } | |
| target = Objects.requireNonNullElse(writeJsonProcessorConfig.getTarget(), source); |
There was a problem hiding this comment.
@KarstenSchnitter, I will keep that in mind for future. For now, I am keeping it as is.
Signed-off-by: Krishna Kondaka <[email protected]>
| @@ -25,7 +26,9 @@ | |||
| @DataPrepperPlugin(name = "write_json", pluginType = Processor.class, pluginConfigurationType = WriteJsonProcessorConfig.class) | |||
| public class WriteJsonProcessor extends AbstractProcessor<Record<Event>, Record<Event>> { | |||
| private static final Logger LOG = LoggerFactory.getLogger(WriteJsonProcessor.class); | |||
| private static final String WRITE_JSON_FAILED_COUNTER = "writeJsonFailedCounter"; | |||
There was a problem hiding this comment.
Nit: could just name this writeJsonEventsFailed. Counter is a bit redundant.
| @@ -48,7 +52,8 @@ public Collection<Record<Event>> doExecute(Collection<Record<Event>> records) { | |||
| try { | |||
| event.put(target, objectMapper.writeValueAsString(value)); | |||
| } catch (Exception e) { | |||
| LOG.error("Failed to convert source to json string"); | |||
| LOG.error("Failed to convert source to json string", e); | |||
| writeJsonFailedCounter.increment(); | |||
There was a problem hiding this comment.
Could add a unit test to cover this case and verify metric is incremented
Description
A processor to write JSON string representation of an object
Issues Resolved
Resolves #832
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.