update!(config): soft deprecation of drop stats counters in `syscall_event_drops` #3015

incertum · 2024-01-16T04:45:48Z

What type of PR is this?

Uncomment one (or more) /kind <> lines:

/kind bug

/kind cleanup

/kind design

/kind documentation

/kind failing-test

/kind feature

/kind release

Any specific area of the project related to this PR?

Uncomment one (or more) /area <> lines:

/area build

/area engine

/area tests

/area proposals

/area CI

What this PR does / why we need it:

Let's discuss deprecating syscall_event_drops.
@falcosecurity/falco-maintainers

Which issue(s) this PR fixes:

#2910

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

update!(config): soft deprecation of drop stats counters in `syscall_event_drops`

Andreagit97 · 2024-01-16T11:34:13Z

userspace/falco/app/actions/load_config.cpp

@@ -41,6 +41,12 @@ static falco::app::run_result apply_deprecated_options(falco::app::state& s)
 		return run_result::fatal("You can not specify more than one of -e, -g (--gvisor-config), --modern-bpf, --nodriver, and the FALCO_BPF_PROBE env var");
 	}

+	if(s.config->m_min_priority == falco_common::PRIORITY_DEBUG)


Personally i had no time to check if we can really drop it, anyone willing to help @falcosecurity/falco-maintainers ?

@leogr encouraged me to just get this PR going and then make the necessary decisions.

Yes, it still is worth investigating a bit since it seems not really used. However, if we are in doubt, we can still postpone this deprecation to 0.38.0 (and remove it in 0.39.0). Let's see if we find any info.

Ok, I took some time to check it, I'm ok with deprecating this! If we agree I would love to do it in Falco 0.37.0 so that in the next dev cycle we can clean up the code!

If we are not sure, I propose to deprecate it now, and if we face some blockers when we try to remove it in the next dev-cycle we can expand the depreciation also for Falco 0.38.0, WDYT? btw I don't think this is the case, I don't see big blockers in removing it

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Andreagit97 · 2024-01-23T12:42:35Z

userspace/falco/app/actions/load_config.cpp

+	if(s.config->m_min_priority == falco_common::PRIORITY_DEBUG)
+	{
+		falco_logger::log(falco_logger::level::WARNING,
+				"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! Use 'metrics' config instead. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");


Suggested change

"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! Use 'metrics' config instead. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");

"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! If you rely on this config use instead 'metrics.output_rule' to monitor the number of drops. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");

I would try to highlight that syscall_event_drops is enabled by default but it probably not many users rely on it, they probably don't even know that is config is doing something under the hood

I like this suggestion! Added.

Andreagit97 · 2024-01-23T12:44:08Z

userspace/falco/app/actions/load_config.cpp

@@ -41,6 +41,12 @@ static falco::app::run_result apply_deprecated_options(falco::app::state& s)
 		return run_result::fatal("You can not specify more than one of -e, -g (--gvisor-config), --modern-bpf, --nodriver, and the FALCO_BPF_PROBE env var");
 	}

+	if(s.config->m_min_priority == falco_common::PRIORITY_DEBUG)


Ok, I took some time to check it, I'm ok with deprecating this! If we agree I would love to do it in Falco 0.37.0 so that in the next dev cycle we can clean up the code!

If we are not sure, I propose to deprecate it now, and if we face some blockers when we try to remove it in the next dev-cycle we can expand the depreciation also for Falco 0.38.0, WDYT? btw I don't think this is the case, I don't see big blockers in removing it

Andreagit97 · 2024-01-23T12:45:41Z

if we go for the deprecation we need to add it to #2763

leogr · 2024-01-23T14:15:53Z

if we go for the deprecation we need to add it to #2763

Since we don't have enough feedback, I will move this to 0.38.

incertum · 2024-01-23T17:40:13Z

+1 re @Andreagit97 assessment, the old drop drop stats also add an avoidable overhead to the current main processing loop. We have an opportunity to clean up the underlying code a lot and I think this will be great as well. In addition, we have help guides and additional documentation around the new metrics currently under review for the website. This is why I believe it could be a good point in time to start the deprecation cycle, at minimum the notice as @Andreagit97 mentioned.

Co-authored-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

add CHANGE NOTICE wrt syscall_event_drops Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

Andreagit97

/approve

poiana · 2024-01-25T08:44:55Z

LGTM label has been added.

Git tree hash: 0e6ce09f18f0c30186a1886c67191b8defec48f4

poiana · 2024-01-25T09:33:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, incertum, 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,incertum,leogr]

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

poiana added release-note kind/cleanup dco-signoff: yes area/engine labels Jan 16, 2024

poiana requested review from Kaizhe and LucaGuerra January 16, 2024 04:45

poiana added approved size/XS labels Jan 16, 2024

Andreagit97 reviewed Jan 16, 2024

View reviewed changes

update!(config): add deprecation notice for syscall_event_drops

d534324

Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the deprecate-old-stats branch from 363ab46 to d534324 Compare January 16, 2024 16:14

poiana added size/S and removed size/XS labels Jan 16, 2024

Andreagit97 added this to the 0.37.0 milestone Jan 17, 2024

Andreagit97 reviewed Jan 23, 2024

View reviewed changes

incertum force-pushed the deprecate-old-stats branch from 294134e to 930c673 Compare January 23, 2024 17:43

cleanup(configs): adjust old stats deprecation notice

9223e1d

Co-authored-by: Andrea Terzolo <andreaterzolo3@gmail.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

incertum force-pushed the deprecate-old-stats branch from 930c673 to 9223e1d Compare January 23, 2024 17:45

update(config): soft deprecation of old stats

0330cee

add CHANGE NOTICE wrt syscall_event_drops Co-authored-by: Leonardo Grasso <me@leonardograsso.com> Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

poiana added size/XS and removed size/S labels Jan 24, 2024

incertum changed the title ~~update!(config): add deprecation notice for syscall_event_drops~~ update!(config): soft deprecation of drop stats counters in syscall_event_drops Jan 24, 2024

Andreagit97 approved these changes Jan 25, 2024

View reviewed changes

poiana assigned Andreagit97 Jan 25, 2024

poiana added the lgtm label Jan 25, 2024

leogr approved these changes Jan 25, 2024

View reviewed changes

poiana assigned leogr Jan 25, 2024

poiana merged commit bb4a643 into falcosecurity:master Jan 25, 2024
28 checks passed

Andreagit97 mentioned this pull request Jan 25, 2024

[TRACKING] Breaking changes in Falco 0.38 #2840

Open

11 tasks

FedeDP mentioned this pull request Jan 26, 2024

sync: release 0.37.x #3035

Merged

Andreagit97 mentioned this pull request Jan 29, 2024

[Tracking] Falco 0.37.0 #3003

Closed

29 tasks

incertum deleted the deprecate-old-stats branch March 5, 2024 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update!(config): soft deprecation of drop stats counters in `syscall_event_drops` #3015

update!(config): soft deprecation of drop stats counters in `syscall_event_drops` #3015

incertum commented Jan 16, 2024 •

edited

Andreagit97 Jan 16, 2024

incertum Jan 16, 2024

leogr Jan 16, 2024

Andreagit97 Jan 23, 2024

Andreagit97 Jan 23, 2024

Andreagit97 Jan 23, 2024

incertum Jan 23, 2024

Andreagit97 Jan 23, 2024

Andreagit97 commented Jan 23, 2024

leogr commented Jan 23, 2024

incertum commented Jan 23, 2024

Andreagit97 left a comment

poiana commented Jan 25, 2024

poiana commented Jan 25, 2024

	"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! Use 'metrics' config instead. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");
	"DEPRECATION NOTICE: 'syscall_event_drops' config is deprecated and will be removed in Falco 0.38! If you rely on this config use instead 'metrics.output_rule' to monitor the number of drops. Note that the 'syscall_event_drops' config is enabled by default when the 'priority' is set to 'debug'. You can turn it off by setting the 'priority' to any higher level\n");

update!(config): soft deprecation of drop stats counters in syscall_event_drops #3015

update!(config): soft deprecation of drop stats counters in syscall_event_drops #3015

Conversation

incertum commented Jan 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 commented Jan 23, 2024

leogr commented Jan 23, 2024

incertum commented Jan 23, 2024

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Jan 25, 2024

poiana commented Jan 25, 2024

update!(config): soft deprecation of drop stats counters in `syscall_event_drops` #3015

update!(config): soft deprecation of drop stats counters in `syscall_event_drops` #3015

incertum commented Jan 16, 2024 •

edited