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

RFC: Error transmission through batching components #11947

Closed
wants to merge 9 commits into from

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Dec 18, 2024

Description

Draft RFC addresses #11308.

Link to tracking issue

Fixes #11308.

Testing

n/a

Documentation

This is a new RFC. As these changes are accepted and made, user-facing
documentation will be added.

@jmacd jmacd requested a review from a team as a code owner December 18, 2024 01:02
@jmacd jmacd requested a review from dmitryax December 18, 2024 01:02
@jmacd jmacd changed the title Jmacd/error transmission rfc RFC: Error transmission through batching components Dec 18, 2024
@jmacd jmacd marked this pull request as draft December 18, 2024 01:03
Copy link

codecov bot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.62%. Comparing base (4593ba7) to head (29f493f).
Report is 231 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11947      +/-   ##
==========================================
+ Coverage   91.59%   91.62%   +0.03%     
==========================================
  Files         449      447       -2     
  Lines       23761    23731      -30     
==========================================
- Hits        21763    21743      -20     
+ Misses       1623     1613      -10     
  Partials      375      375              

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

@jmacd
Copy link
Contributor Author

jmacd commented Dec 19, 2024

I will present this RFC in the next two Collector SIG meetings 1/7/2025 (APAC/PT) and 1/15/2025 (NA).

@jmacd jmacd marked this pull request as ready for review December 19, 2024 19:07
Copy link
Contributor

github-actions bot commented Jan 3, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added Stale and removed Stale labels Jan 3, 2025
Comment on lines +126 to +131
Concurrency behavior varies. In the case where `MergeSplit()` is used, there is
a potential for multiple requests to emit from a single request. In this case,
steps 4 and 5 are executed repeatedly while there are more requests, meaning:

1. Exports are synchronous and sequential.
2. An error causes aborting of subsequent parts of the request.
Copy link
Member

Choose a reason for hiding this comment

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

This is not the case anymore. This was resolved as part of #10368. All the split requests are sent concurrently now

Comment on lines +150 to +151
to enqueue the request. If the queue is full, the queue sender returns a queue-full
error immediately.
Copy link
Member

Choose a reason for hiding this comment

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

It's now configurable to block the caller if the queue is full

behavior that leads to data loss. This requires one or more changes
in the exporterhelper:

1. Disable the queue sender by default. In this configuration, requests
Copy link
Member

Choose a reason for hiding this comment

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

This is a significant change in the behavior. I don't remember any other user requests to make the in-memory queue disabled by default. I believe a significant portion of users would favor keeping in-memory queue enabled by default, even if it's prone to data loss in case of Collector's crash. If we want to change that, there should be a more extended discussion and evaluation. AFAIR, in-memory buffering is a common default setting in other data processing agents.

Comment on lines +210 to +212
- Recognize partial splitting. When a request is split, leaving
part of a request that is not full, it remains in the active
request (i.e., still pending).
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid this might introduce a significant delay when several incoming requests that are partially split block each other until the timeout is reached.

Copy link
Contributor

github-actions bot commented Feb 1, 2025

This PR was marked stale due to lack of activity. It will be closed in 14 days.

@github-actions github-actions bot added the Stale label Feb 1, 2025
@jmacd
Copy link
Contributor Author

jmacd commented Feb 3, 2025

This work is out of date since #12245, #12233, #12118, #12154, #12246, #12242, and so on.

I believe all of the above work deserves an RFC.

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

Successfully merging this pull request may close these issues.

Batch processor concurrency and error transmission
2 participants