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

Change memorylimiter and queue full errors to carry time-after information #12368

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bogdandrutu
Copy link
Member

@bogdandrutu bogdandrutu commented Feb 12, 2025

Depends on #12367

Copy link

codecov bot commented Feb 12, 2025

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 91.53%. Comparing base (0831231) to head (3924197).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
exporter/exporterqueue/queue.go 75.00% 1 Missing and 1 partial ⚠️
internal/memorylimiter/memorylimiter.go 75.00% 1 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (75.00%) is below the target coverage (95.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #12368      +/-   ##
==========================================
- Coverage   91.54%   91.53%   -0.02%     
==========================================
  Files         467      467              
  Lines       25623    25634      +11     
==========================================
+ Hits        23456    23463       +7     
- Misses       1768     1770       +2     
- Partials      399      401       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bogdandrutu bogdandrutu force-pushed the use-retry-after-errors branch from 4afe9ab to 6d04ffc Compare February 14, 2025 22:44
@bogdandrutu bogdandrutu changed the title Use retry after errors Change memorylimiter and queue full errors to carry time-after information Feb 14, 2025
@bogdandrutu bogdandrutu force-pushed the use-retry-after-errors branch from 6d04ffc to 3924197 Compare February 15, 2025 04:13
var ErrQueueIsFull = func() error {
st := status.New(codes.ResourceExhausted, "sending queue is full")
dt, err := st.WithDetails(&errdetails.RetryInfo{
RetryDelay: durationpb.New(1 * time.Second),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

Make ErrQueueIsFull a struct type supporting the Golang errors package's Is() and As() methods, and let the OTLP receiver translate this into a status.New(codes.ResourceExhausted, err.Error()) and the WithDetails parts itself.

type QueueIsFullError struct {
  Duration time.Duration
} 
func (e QueueIsFullError) Error() string {
  return fmt.Sprintf("sending queue is full: retry after %v", e.Duration)
}
var ErrQueueIsFull = QueueIsFullError{time.Second}

Then, allow the component to configure a specific retry-after constant via the Config struct. At runtime, inside the component, use the configured retry-after and store a queueFullErr field in the component, then the user can configure this value (e.g., self.queueFullErr = QueueIsFullError{cfg.RetryAfter}. The OTLP receiver will use e.g.,

  err := pipe.Consume(ctx, request)
  candidate := exporterqueue.QueueIsFullError{}
  if errors.As(err, &candidate) {
    gst := status.New(codes.ResourceExhausted, ...)
    if dt, err := gst.withDetails(...)
       gst = dt
    }
    return gst.Err()
  }
  // ...

 

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that this is not a scalable solution that you are proposing unless we say there is only one error like this that we support.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is because if we start adding checks for 10s of types of errors in every receiver this will become a big problem to maintain.

Comment on lines +29 to +31
if err != nil {
panic(err)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if err != nil {
panic(err)
}

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

Successfully merging this pull request may close these issues.

3 participants