- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.8k
consumererror: Add partial error type #13927
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
base: main
Are you sure you want to change the base?
Conversation
39ad8a4    to
    8faf1e8      
    Compare
  
    This PR adds functionality for consumers to create a partial error type. This will allow consumers to properly report partial success/failure with failed item counts, which can subsequently be used when reporting sent/failed metrics.
8faf1e8    to
    e20fe3c      
    Compare
  
    | Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@            Coverage Diff             @@
##             main   #13927      +/-   ##
==========================================
- Coverage   91.66%   91.66%   -0.01%     
==========================================
  Files         652      653       +1     
  Lines       42516    42531      +15     
==========================================
+ Hits        38973    38985      +12     
- Misses       2734     2736       +2     
- Partials      809      810       +1     ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a lot of background on this module so no strong opinion on the API itself, but I do feel like the two items I commented below should be addressed before moving forward
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather have this in xconsumererror at the start, use it in contrib and once we are confident about it we move it to the 'stable' module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a testable example on how to use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see a draft, at least, for the documentation that will accompany the package, what the consumers need to know. For example, in the fanout consumer, existing logic returns immediately when one of the fanned-out consumers returns a non-nil error. I believe we need a blanket recommendation that these errors may be treated the same as success in cases like these.
However even while continuing the fan-out, the returned error should join all the partial successes with good information that an OTLP receiver can convey to SDKs and user consoles. Here are some general rules that might make sense to apply:
- When applying the same request multiple times: concatenate partial-success message strings, take the maximum number of rejected items
- When combining requests as in a batch processor, prepend "batch rejected %d items: " to the partial-success message, set number rejected to 0
| This PR was marked stale due to lack of activity. It will be closed in 14 days. | 
Description
This PR adds functionality for consumers to create a partial error type. This will allow consumers to properly report partial success/failure with failed item counts, which can subsequently be used when reporting sent/failed metrics.
Link to tracking issue
Part of #13423
Testing
Documentation
godoccomments are added to the new public functions inconsumererror.