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

chore: clone the context instead of using background context while pushing logs to ingesters from distributor #15735

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

sandeepsukhani
Copy link
Contributor

What this PR does / why we need it:
We use background context to push logs to Ingesters from Distributors to continue to ingest logs when we return early. This is to avoid stopping ingestion to remaining ingesters after hitting quorum. However, we have to explicitly copy the required context keys from the parent to the newly created context.

I am working on some changes that add a new key to the context, and I feel that copying all the required keys is cumbersome and error-prone. What we want here is a clone of context without cancellation propagation from parent to child. This is now achievable using context.WithoutCancel.

…ogs to ingesters from distributor
@sandeepsukhani sandeepsukhani requested a review from a team as a code owner January 14, 2025 12:28
Comment on lines +703 to +705
// Clone the context using WithoutCancel, which is not canceled when parent is canceled.
// This is to make sure all ingesters get samples even if we return early
localCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), d.clientCfg.RemoteTimeout)
Copy link
Contributor

@DylanGuedes DylanGuedes Jan 14, 2025

Choose a reason for hiding this comment

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

just one question, we were already using context.Background(), doesn't that mean it doesn't have a parent already?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is right. However, it creates a fresh context without any keys, so it requires us to copy all the keys needed in our code, like tenant ID.

Copy link
Contributor

@chaudum chaudum left a comment

Choose a reason for hiding this comment

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

👍

@sandeepsukhani sandeepsukhani merged commit 1130542 into main Jan 14, 2025
61 checks passed
@sandeepsukhani sandeepsukhani deleted the ingester-push-clone-context branch January 14, 2025 14:22
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.

None yet

3 participants