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

Complete writing a batch even if some spans refused #1957

Closed
joe-elliott opened this issue Dec 15, 2022 · 8 comments · Fixed by #2571
Closed

Complete writing a batch even if some spans refused #1957

joe-elliott opened this issue Dec 15, 2022 · 8 comments · Fixed by #2571
Labels
keepalive Label to exempt Issues / PRs from stale workflow type/bug Something isn't working
Milestone

Comments

@joe-elliott
Copy link
Member

joe-elliott commented Dec 15, 2022

Describe the bug
Currently if an ingester refuses traces b/c a trace is too large it will stop writing the current batch and return an error. The end result is that the first part of the batch was written but everything after the "trace too large" error is dropped. Let's continue writing the batch even if we receive a trace too large error.

Loop to write traces:

for j := range req.Traces {

Trace too large returned here:

return status.Errorf(codes.FailedPrecondition, (newTraceTooLargeError(id, i.instanceID, maxBytes, reqSize).Error()))

@joe-elliott joe-elliott added type/bug Something isn't working good first issue Good for newcomers labels Dec 15, 2022
@joe-elliott joe-elliott added this to the v2.1 milestone Dec 15, 2022
@97amarnathk
Copy link

@joe-elliott can I pick this up ?

@joe-elliott
Copy link
Member Author

Absolutely! The team is going to be pretty barebones for the next 2 weeks due to the holidays so if you don't get an immediate review don't worry. I promise I will get to all PRs when I get back.

Thanks for giving it a shot.

@97amarnathk
Copy link

@joe-elliott 1 small implementation doubt.

Since this is now a bulk operation how should the ingester convey back the traces which failed ?

@mapno
Copy link
Member

mapno commented Dec 19, 2022

Hi @97amarnathk!

Since this is now a bulk operation how should the ingester convey back the traces which failed ?

The error in non-retryable, so we're good returning a list of the traces that failed (i.e. similar to what we already do, but for multiple traces). I don't think we need a more comprehensive approach than that.

@97amarnathk
Copy link

Got it.
I have implemented multierror handling for this here: #1962

Do go through it whenever possible

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added the stale Used for stale issues / PRs label Feb 18, 2023
@bilbof
Copy link

bilbof commented Feb 19, 2023

keepalive

@github-actions github-actions bot removed the stale Used for stale issues / PRs label Feb 20, 2023
@mapno mapno added the keepalive Label to exempt Issues / PRs from stale workflow label Feb 22, 2023
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had any activity in the past 60 days.
The next time this stale check runs, the stale label will be removed if there is new activity. The issue will be closed after 15 days if there is no new activity.
Please apply keepalive label to exempt this Issue.

@github-actions github-actions bot added stale Used for stale issues / PRs and removed stale Used for stale issues / PRs labels Oct 12, 2023
@joe-elliott joe-elliott removed the good first issue Good for newcomers label Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keepalive Label to exempt Issues / PRs from stale workflow type/bug Something isn't working
Projects
Status: Done
4 participants