Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide additional metadata in errors #957

Open
eliw00d opened this issue Nov 12, 2020 · 7 comments · May be fixed by #1657
Open

Provide additional metadata in errors #957

eliw00d opened this issue Nov 12, 2020 · 7 comments · May be fixed by #1657

Comments

@eliw00d
Copy link
Contributor

eliw00d commented Nov 12, 2020

Is your feature request related to a problem? Please describe.
Currently, there are many errors that do not provide certain metadata. For example, TOPIC_AUTHORIZATION_FAILED does not provide the topic(s) that failed to authorize. We provide our producer with multiple brokers and use sendBatch to send to multiple topics. It appears that some messages go through to one broker but not another but a lot of research needs to be done to figure out the problematic topic(s). It would be nice if the error message provided more information for diagnosing issues.

Describe the solution you'd like
For error messages related to producing or consuming from topics it would be useful to have the topics that caused the error in the error.

@Nevon
Copy link
Collaborator

Nevon commented Nov 16, 2020

This is a good idea and something that can be done incrementally on a case-by-case basis.

The way that protocol errors work is that whenever we receive a response from the API, there can be one or more error codes returned. For example, the response for CreateTopics v0 looks like this:

CreateTopics Response (Version: 0) => [topics] 
  topics => name error_code 
    name => STRING
    error_code => INT16

This can be read as an array of topics, where each topics has two properties: name (string) and error_code (int16).

The error code can then be looked up in this list. So if we for example receive the number 29, that corresponds to TOPIC_AUTHORIZATION_FAILED.

In most cases, this is handled generically using the createErrorFromCode function:

const createErrorFromCode = code => {
return new KafkaJSProtocolError(errorCodes.find(e => e.code === code) || unknownErrorCode(code))
}

CreateTopics is a typical example of how this is done:

const parse = async data => {
const topicsWithError = data.topicErrors.filter(({ errorCode }) => failure(errorCode))
if (topicsWithError.length > 0) {
throw createErrorFromCode(topicsWithError[0].errorCode)
}
return data
}

In this case, we are simply checking all the topics in the returned list, and if there's any error, we throw the first one we find. Obviously we could do a much better job by creating a specific error class that extends the protocol error with more information, and also aggregates all of the errors rather than just picking the first one.

@prithvijit-dasgupta
Copy link
Contributor

@Nevon From what I gather from the issue it seems like that each of the protocol calls would need a separate wrapped error class (depending on the use case).
I am very much interested in helping with this project as I will be using this library in our company's Kafka consumers.

@Nevon
Copy link
Collaborator

Nevon commented May 17, 2021

I think it would have to be on a case-by-case basis. In some cases, it might be enough to create a class for a specific protocol error (this is already done in some cases) and throw that instead of a generic KafkaJSProtocolError. In some other cases, you might need to create a wrapping error class per protocol, as you mention. For example, CreateTopics probably needs a specific KafkaJSCreateTopicError that contains a KafkaJSProtocolError per topic, or something of that nature.

There's also looking into where these errors are caught. I suspect there are a few places where we are explicitly handling KafkaJSProtocolError that may need to be extended to also cover for more specific errors, if we break them out into their own error classes.

@prithvijit-dasgupta
Copy link
Contributor

I'll give a initial try with wrapping class approach on the CreateTopic protocol. Will try to address the issues and discuss them here.

@prithvijit-dasgupta
Copy link
Contributor

Next phase for restructure I am targeting (each point will be a separate PR)

  1. consumer message API errors
  2. consumer connection, rebalance, heartbeat API errors
  3. other consumer related errors

After that probably I will be targeting the producer module

@guikubivan
Copy link

The changes made in #1104 unfortunately don't include the actual error message from bubbling up when creating topic. I had to use the debugger to stop execution and inspect the error message around this line:
https://github.com/prithvijit-dasgupta/kafkajs/blob/master/src/protocol/requests/createTopics/v0/response.js#L32

@Papooch
Copy link

Papooch commented Apr 24, 2023

What is the status of this issue? The affecting topic is still not available in the KafkaJSProtocolError. Is there any userland solution to retrieve it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants