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

fix: Syslog Agent HTTPS drains reuse and release fasthttp requests and responses where appropriate #573

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ctlong
Copy link
Member

@ctlong ctlong commented May 3, 2024

Syslog Agent uses fasthttp under the hood for HTTPS drains. When making HTTPS requests to drains to send syslog messages we use AcquireRequest and AcquireResponse, which we can reuse and eventually release.

Hopefully this change has a positive improvement on the performance of HTTPS drains.

ctlong added 2 commits May 6, 2024 22:05
Reuses fasthttp requests and responses within the writer as appropriate,
and releases them afterwards.

Also includes some light refactoring for clarity.

Signed-off-by: Carson Long <12767276+ctlong@users.noreply.github.com>
@ctlong ctlong changed the title fix: Syslog Agent HTTPS drains release requests and responses appropriately fix: Syslog Agent HTTPS drains reuse and release fasthttp requests and responses where appropriate May 6, 2024
@ctlong ctlong self-assigned this May 6, 2024
@ctlong ctlong marked this pull request as draft May 6, 2024 23:04
@ctlong
Copy link
Member Author

ctlong commented May 6, 2024

I want to run some manual tests on this before merging.

@nicklas-dohrn
Copy link

I think this would help in many cases, but only if there are multiple messages present in one msgs batch.
From my understanding of the flow of events through loggregator, this is highly inhomogeneous, so factoring out the
aquiry of request objects regardless of the messages might prove more powerful 🤔

But that change would lead to more complicated request aquirations, so I am torn if that would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Pending Review | Discussion
Development

Successfully merging this pull request may close these issues.

None yet

2 participants