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.

Sign up for GitHub

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

Jump to bottom

cleanup(userspace/falco)!: improvements to the http output perf. #2602

Merged
merged 2 commits into from
Aug 31, 2023
Merged

cleanup(userspace/falco)!: improvements to the http output perf. #2602

merged 2 commits into from
Aug 31, 2023

Conversation

FedeDP
Copy link
Contributor

@FedeDP FedeDP commented Jun 1, 2023 •
edited by leogr
Loading

What type of PR is this?

/kind cleanup

Any specific area of the project related to this PR?

/area engine

What this PR does / why we need it:

Improves http_output performance by creating the curl handle just once with all its options; then it's just a matter of performing the call with the correct post fields.

Moreover, add an option to disable server answer echoing to stdout.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

chore: improved HTTP output performance
update!: HTTP output will no more echo to stdout by default
new: added option to enable/disable echoing of server answer to stdout (disabled by default) when using HTTP output

@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from 891819a to 7f277b6 Compare June 1, 2023 09:25
@poiana poiana requested review from Andreagit97 and Kaizhe June 1, 2023 09:25
@poiana poiana added the size/L label Jun 1, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 1, 2023

/milestone 0.36.0

@poiana poiana added this to the 0.36.0 milestone Jun 1, 2023
@FedeDP FedeDP changed the title cleanup(userspace/falco): improvements to the http output perf. wip: cleanup(userspace/falco): improvements to the http output perf. Jun 1, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 1, 2023

We also need to support output reloading! TODO.

@FedeDP FedeDP changed the title wip: cleanup(userspace/falco): improvements to the http output perf. cleanup(userspace/falco): improvements to the http output perf. Jun 1, 2023
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 1, 2023

We also need to support output reloading! TODO.

No problem, really. We don't need to support http output reloading since it is just a "close file open file" behavior (it is not linked to any config update).

@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from 7f277b6 to edc88d9 Compare June 29, 2023 08:07
@FedeDP
Copy link
Contributor Author

FedeDP commented Jun 29, 2023

Rebased on top of master.

FedeDP
FedeDP commented Aug 23, 2023
View reviewed changes
falco.yaml Outdated Show resolved Hide resolved
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 28, 2023

/cc @leogr original author of the http output :)

Andreagit97
Andreagit97 reviewed Aug 28, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

Thank you for this! Some comments about the style and one more important comment about what we want to do with "echo" config

falco.yaml Outdated Show resolved Hide resolved
userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
@Andreagit97
Copy link
Member

Still asking myself why we initialized curl at every iteration... are we missing something? @leogr

@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from edc88d9 to 7f18061 Compare August 30, 2023 13:34
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 30, 2023

Rebased and fixed up @Andreagit97 suggestions!

@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from 7f18061 to 40130bc Compare August 30, 2023 13:36
@Andreagit97 Andreagit97 changed the title cleanup(userspace/falco): improvements to the http output perf. cleanup(userspace/falco)!: improvements to the http output perf. Aug 30, 2023
Andreagit97
Andreagit97 reviewed Aug 30, 2023
View reviewed changes
submodules/falcosecurity-rules Outdated
Copy link
Member

Choose a reason for hiding this comment

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

i don't think we need this :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ouch :/

@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from 40130bc to 5d9595d Compare August 30, 2023 14:27
@Andreagit97
Copy link
Member

IMO this is ready, could you rebase please? @FedeDP

@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 31, 2023

:( i hate falcosecurity-rules submodule @jasondellaluce 😆

@FedeDP
cleanup(userspace/falco): improvements to the http output perf.
37f1c95 
Moreover, add option to disable stdout echoing.

Signed-off-by: Federico Di Pierro <[email protected]>
@FedeDP FedeDP force-pushed the cleanup/http_output_improvements branch from 5d9595d to 37f1c95 Compare August 31, 2023 10:01
@FedeDP
Copy link
Contributor Author

FedeDP commented Aug 31, 2023

Bumped rules to latest master version.

Andreagit97
Andreagit97 previously approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana poiana added the lgtm label Aug 31, 2023
@leogr
Copy link
Member

leogr commented Aug 31, 2023

Still asking myself why we initialized curl at every iteration... are we missing something? @leogr

@Andreagit97
The original design of the http_out likely assumed that dispose curl at every iteration was safer to avoid memory leak and the performance cost was acceptable since alerts should usually have a low frequency. This is just a speculation since, when I refactored the whole output system, I just ported the same approach (the refactor was already too big also to introduce too many improvements).

Thus, we should have everything ok.

@Andreagit97
Copy link
Member

cool! thank you for the explanation :)

leogr
leogr reviewed Aug 31, 2023
View reviewed changes
Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Just left a comment, otherwise SGTM!

userspace/falco/outputs_http.cpp Outdated Show resolved Hide resolved
@FedeDP @leogr
chore(userspace/falco): properly check that parent init() did not fai…
88d2886 
…l for reasons.

Signed-off-by: Federico Di Pierro <[email protected]>

Co-authored-by: Leonardo Grasso <[email protected]>
Andreagit97
Andreagit97 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@Andreagit97 Andreagit97 left a comment

Choose a reason for hiding this comment

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

/approve

@poiana
Copy link
Contributor

poiana commented Aug 31, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [Andreagit97,FedeDP,leogr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@poiana poiana merged commit 26f626c into master Aug 31, 2023
15 checks passed
@poiana poiana deleted the cleanup/http_output_improvements branch August 31, 2023 14:11
@LucaGuerra LucaGuerra mentioned this pull request Sep 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andreagit97 Andreagit97 Andreagit97 approved these changes

@leogr leogr leogr approved these changes

@Kaizhe Kaizhe Awaiting requested review from Kaizhe

Assignees

@leogr leogr

@Andreagit97 Andreagit97

Labels
approved area/engine dco-signoff: yes kind/cleanup lgtm release-note size/L
Projects
Falco Roadmap
Archived in project
Milestone
0.36.0
Development

Successfully merging this pull request may close these issues.
4 participants
@FedeDP @Andreagit97 @leogr @poiana