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

Investigate how to expose exporterhelper.NewThrottleRetry in the consumererror #7047

Open
bogdandrutu opened this issue Jan 27, 2023 · 3 comments · May be fixed by #11085
Open

Investigate how to expose exporterhelper.NewThrottleRetry in the consumererror #7047

bogdandrutu opened this issue Jan 27, 2023 · 3 comments · May be fixed by #11085
Assignees

Comments

@bogdandrutu
Copy link
Member

No description provided.

@bogdandrutu
Copy link
Member Author

An idea is to merge the current consumererror.[Traces|Metrics|Logs] with the ThrottleRetry and have a

// which contains the data that need to be retried, and the potential `delay` if any.
type consumererror.Retriable[Traces|Metrics|Logs] 

I am not sure we need an error per signal or we can use generics, or simply have all possible signals as part of the error. Both these options will create a consumererror.Retriable type which has a nice name.

If we do this, we should also include a "failed" uint64 to the consumererror.Permanent that represents the number of signal entries (spans/dps (or metrics)/log records) dropped. The reason to not return like the Retirable the pdata is because this is permanent error, so it is useless to filter/construct a pdata with the wrong data.

@dashpole
Copy link
Contributor

Side note: the googlecloud exporter likely won't utilize this. The underlying client libraries have relatively intelligent retry mechanisms built-in, and we are in the process of making a fix to the trace retry logic before we switch away from the collector's retry mechanism.

But in general, I think #7047 (comment) makes a lot of sense.

@evan-bradley
Copy link
Contributor

I am not sure we need an error per signal or we can use generics, or simply have all possible signals as part of the error.

I think an error type per signal that is aliased or extended from a generic base class would keep duplication to a minimum while not requiring users to instantiate the generic types. Just including all possible signals could work, but most types are specific to a signal, so this seems unconventional. Are there any other places where this is done?

If we do this, we should also include a "failed" uint64 to the consumererror.Permanent that represents the number of signal entries (spans/dps (or metrics)/log records) dropped.

I agree about adding a failed field. Are there other fields we would want? One that that comes to mind is a total that counts of the total number of telemetry items sent in the payload, or alternatively a successful to count successful items. Another would be a reason for failure (rejected by backend, backend is down, processing failure, etc.). These are both immediately useful for OTLP partial success responses, but I think would be broadly useful for any component.

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