feat: clarify key or value in (de)serialization processing log messages #6109
Conversation
| @@ -121,6 +121,11 @@ message.deserializationError (STRUCT) | |||
| : The contents of a message with type 0 (DESERIALIZATION_ERROR). | |||
| Logged when a deserializer fails to deserialize an {{ site.ak }} record. | |||
|
|
|||
| message.deserializationError.component (STRING) | |||
There was a problem hiding this comment.
Not a huge fan of the name "component" but couldn't think of anything better. Hopefully someone else has a better suggestion?
There was a problem hiding this comment.
I poked around a bit and didn't see consensus on a good term, but I did see a few sources using the very technical term "part", like "key-part" and "value-part".
There was a problem hiding this comment.
Don't mind component or part, or how about target?
There was a problem hiding this comment.
Sure, updated to "target".
| @@ -121,6 +121,11 @@ message.deserializationError (STRUCT) | |||
| : The contents of a message with type 0 (DESERIALIZATION_ERROR). | |||
| Logged when a deserializer fails to deserialize an {{ site.ak }} record. | |||
|
|
|||
| message.deserializationError.component (STRING) | |||
There was a problem hiding this comment.
Don't mind component or part, or how about target?
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(exception, record, topic); | ||
| return Objects.hash(exception, record, topic, isKey); |
There was a problem hiding this comment.
We should have a unit test testing this. Can you add one please?
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| return Objects.hash(exception, record, topic); | ||
| return Objects.hash(exception, record, topic, isKey); |
There was a problem hiding this comment.
We should have a unit test testing this. Can you add one please?
|
|
||
| static List<String> getCause(final Throwable exception) { | ||
| final List<String> cause = ErrorMessageUtil.getErrorMessages(exception); | ||
| cause.remove(0); |
There was a problem hiding this comment.
I know this is a cut & paste, but I've seen this remove(0) remove the actual cause of the error, e.g. I've seen cases where exception is a simple NullPointerException and the remove(0) has resulted in an empty cause list.
Personally, I'd remove the remove and inline this method.
Which would only leave getRecordComponent, which I'd also be tempted to remove, and just add appropriate "key" and "value" constants to ProcessingLogMessageSchema instead.
There was a problem hiding this comment.
The first error message (the one being removed by this line) is already captured in the errorMessage field, which is why it's purposefully left out of the cause field. It sounds like you're advocating for duplication of the initial error message, so that the cause field is complete?
| @@ -60,6 +61,7 @@ | |||
| @Before | |||
| public void setUp() { | |||
| deserializer = new LoggingDeserializer<>(delegate, processingLogger); | |||
| deserializer.configure(Collections.emptyMap(), false); | |||
There was a problem hiding this comment.
Probably worth adding a test that called configure with isKey=true, and ensure logged error has isKey=true.
Same for LoggingSerializerTest.
Description
Fixes #5627
Currently, there's nothing in the processing log message for (de)serialization errors to indicate whether it was the record key or value that failed to serialize/deserialize. This PR adds a new field to the relevant processing log message schemas called "component" with value "key" or "value" to indicate so.
Not in love with the name "component" but couldn't think of anything better. Suggestions appreciated!
Also fixes a typo in the Kafka deserializer error message that indicated the wrong format (delimited).
Testing done
Unit + manual.
Reviewer checklist