`EncodeError/DecodeError` should be returned with error message

[iamazy]

EncodeError/DecodeError should be returned with error message, otherwise it will be hard to find out exactly where the error occured

[tychedelia]

I agree, this has been a frustration of mine before. Happy to accepts PRs here.

[tychedelia]

For anyone looking to contribute, this would be to modify protocol_codegen/src/generate_messages/generate.rs to add some context every time we write Err(EncodeError) or Err(DecodeError). I'm not sure about how much information is best to include here, but minimally we could at least start with the name of the struct that's implementing Encodable. In some cases we are emitting error logs and might be able to use that as a message. Determining which field failed to serialize might be a bit more involved, so happy to accept a MVP here.

[iamazy]

Hi, @tychedelia, Thanks for your reminding. In my case, It is always caused by this code that makes mistakes without knowing why.

kafka-protocol-rs/src/protocol/mod.rs

Lines 40 to 57 in 13acfb5

	impl From<NotEnoughBytesError> for DecodeError {
	fn from(_: NotEnoughBytesError) -> Self {
	DecodeError
	}
	}
	impl From<Utf8Error> for DecodeError {
	fn from(_: Utf8Error) -> Self {
	DecodeError
	}
	}
	impl From<FromUtf8Error> for DecodeError {
	fn from(_: FromUtf8Error) -> Self {
	DecodeError
	}
	}

It seems that some logs should also be added here.

[tychedelia]

Got it, this would definitely be an easy place to add some more context.

[iamazy]

For anyone looking to contribute, this would be to modify protocol_codegen/src/generate_messages/generate.rs to add some context every time we write Err(EncodeError) or Err(DecodeError). I'm not sure about how much information is best to include here, but minimally we could at least start with the name of the struct that's implementing Encodable. In some cases we are emitting error logs and might be able to use that as a message. Determining which field failed to serialize might be a bit more involved, so happy to accept a MVP here.

I'm not sure changes must be made here, since every return Err(DecodeError) in the protocol_codegen/src/generate_messages/generate.rs and src/protocol/types.rs both have a error log on the previous line

[tychedelia]

For anyone looking to contribute, this would be to modify protocol_codegen/src/generate_messages/generate.rs to add some context every time we write Err(EncodeError) or Err(DecodeError). I'm not sure about how much information is best to include here, but minimally we could at least start with the name of the struct that's implementing Encodable. In some cases we are emitting error logs and might be able to use that as a message. Determining which field failed to serialize might be a bit more involved, so happy to accept a MVP here.

I'm not sure changes must be made here, since every return Err(DecodeError) in the protocol_codegen/src/generate_messages/generate.rs and src/protocol/types.rs both have a error log on the previous line

We're definitely doing an okay job right now with logs, but I think we could be doing better job with adding context to the error types themselves. For example, I'm working on building a terminal UI for Kafka, and so it might be helpful to provide errors to UI when something went wrong. I definitely agree with you about the missing logs though, will try to get to that sometime soon. Thanks :)

[rukai]

I'm going to take this on.

[rukai]

My plan is:

1. replace EncodeError and DecodeError with anyhow::Error
2. replace tracing::error!("useful error); return Err(DecodeError) with return Err(anyhow!("useful error"))

For DecodeError I feel this makes perfect sense as there is no difference in severity or semantic meaning between the errors for the application.
The errors are purely to be logged or included in a panic so the developer can examine them and fix the issue.
Anyhow will also easily let us build up context via the .context() method which could allow us to generate more meaningful errors in the future.

For encoding, an argument could be made that we should have a typed error so that if the user creates an invalid message (too many elements in a collection, field is not available in this version etc.) then they can catch that and back out and try again.
But I would argue that is the incorrect approach and the user should have these checks built into message generation.
Handling this at the point of encoding is a huge code smell.