[consumer] Add retryable error types and add counts to permanent errors #7439
Conversation
There was a problem hiding this comment.
LGTM. Given the conversation we had today, I think there are a few things that can be changed here:
- add the
toHTTPfrom Propagate errors from exporters to receivers #7486 somewhere here, so that gRPC <-> HTTP translation can happen - add new constructors to be used with exporters, like
NewFromGRPCStatus(*status.Status)andNewFromHTTPStatus(int). Perhaps also the other way around would make sense, to be used in receivers:ToHTTPStatus()andToGRPCStatus().
|
@evan-bradley, is this PR going to incorporate parts of #7486? I'm waiting to proceed with that PR under the assumption that parts of it will be incorporated here. |
|
@jpkrohling Yes, that was my intent. Sorry for the delay, I was looking into that and got stopped just short of being able to implement it. I hope to have something this week. |
| // NewPermanentWithCount wraps an error to indicate that it is a permanent error, i.e. an | ||
| // error that will be always returned if its source receives the same inputs. | ||
| // The error should also include the number of rejected records. | ||
| func NewPermanentWithCount(err error, rejected int) error { |
There was a problem hiding this comment.
I'm a bit worried that we require providing the rejected field here. I'm wondering if we can keep this field optional. If not provided, the caller can assume that all the items got rejected. My concern is that some exporters return this error when there is no access to the original pdata that was translated to some other schema. It'll be difficult for them to fetch the number of the items. One option is to have
NewPermanent(err error, otps ...PermanentErrorOptions)
and supply rejected via WithRejectedCount(int) option.
Or we don't even need Permanent part here and assume it's always permanent unless Retryable(ptrace.Traces) option is provided. Later we might have some other options.
These are just thoughts, not suggestions for changes. I believe we need to think more about an overarching design for this API that would cover all the use cases, including embedding the GRPC/HTTP and possible future additions. Maybe having some small design doc would be beneficial.
There was a problem hiding this comment.
I agree that having a design doc would be good, I'll think through some of your points and add them to a design doc here. If it looks like the PR is growing we can move the document to a separate PR.
|
Folks, what's the state of this? This is blocking #7486 and I would really love to move forward with that, perhaps even with a suboptimal solution at first, and a proper solution in the longer term. |
|
@jpkrohling Sorry, I had to focus on a few other things and haven't made as much progress on this as I would have liked. In my opinion, if we can keep the errors you have in #7486 internal, we could move to a long-term solution in the OTLP receiver and exporter once we've agreed on a design here. That way the functionality wouldn't change from user's perspective (with respect to error code propagation) and we wouldn't have to make any breaking changes in the API. What do you think? |
|
Sounds good -- that error is internal already anyway, usable only by collector components in the core repository. Or do you mean making them even more private? |
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
Co-authored-by: Dmitrii Anoshin <anoshindx@gmail.com>
I think the internal scope you have in that PR is good. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
|
Reopening so it's still on our radar |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
@dmitryax thanks for re-opening this. I began the design document that we discussed, but I feel that it is still woefully inadequate, and haven't been able to get it to a spot where I feel it's really ready for a review. If you take a look feel free to let me know if I'm totally off-track, otherwise I hope to improve it in the coming weeks and request another review. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
This will replace
exporterhelper.throttleRetrywithconsumererror.Retryable[Traces|Metrics|Logs]types and add success/fail counts to the permanent error types.Right now this PR is just intended to be a discussion point for how these types should look. I will complete a full implementation once there is consensus on how these should look.
Link to tracking Issue:
Resolves #7047