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

enhancement(http sink): Retrying the HTTP sink in case of 404s and request timeouts #21457

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

noble-varghese
Copy link

@noble-varghese noble-varghese commented Oct 8, 2024

Adding the functionality to retry the log sending on getting a not found error(404), forbidden request (403) or a request timeout (408) from the HTTP sink. Since the defualt behavior is to drop the events during such events, this would affect the log propagation.

solves: #10870

@bits-bot
Copy link

bits-bot commented Oct 8, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the domain: sinks Anything related to the Vector's sinks label Oct 8, 2024
@noble-varghese noble-varghese changed the title enhancement(sinks): Retrying the HTTP sink in case of 404s and request timeouts enhancement(http sink): Retrying the HTTP sink in case of 404s and request timeouts Oct 8, 2024
@noble-varghese noble-varghese marked this pull request as ready for review October 8, 2024 20:51
@noble-varghese noble-varghese requested a review from a team as a code owner October 8, 2024 20:51
@pront
Copy link
Contributor

pront commented Oct 8, 2024

Hi @noble-varghese, thank you for the contribution. I think retrying for these codes is reasonable, I will take a closer look tomorrow.

Note: Need to update the docs as well. See https://vector.dev/docs/reference/configuration/sinks/http/#retry-policy.

@noble-varghese noble-varghese requested review from a team as code owners October 9, 2024 03:30
@github-actions github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Oct 9, 2024
@noble-varghese
Copy link
Author

@pront Thanks for pointing it out ! I have updated the docs page and added unit tests on the http utils as well :)

Copy link
Contributor

@pront pront left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@pront pront left a comment

Choose a reason for hiding this comment

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

Hey @noble-varghese, I noticed the E2E test http_to_http failed. This change actually is a breaking one. Since it is hard to evaluate the risk, it is better to err on the side of caution. After some discussion with the team, I created #21469 to layout a solution for this.

@pront
Copy link
Contributor

pront commented Oct 9, 2024

To unblock this PR, perhaps we can add 408 to the defaults? Still a breaking change, but seems desirable and low risk. cc @jszwedko

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: external docs Anything related to Vector's external, public documentation domain: sinks Anything related to the Vector's sinks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants