feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml #2739

incertum · 2023-08-23T23:04:33Z

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:

As scheduled deprecate stats command args option in favor of metrics configs in falco.yaml for Falco 0.36 release.

@jasondellaluce

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml

github-actions · 2023-08-23T23:04:55Z

This PR may bring feature or behavior changes in the Falco engine and may require the engine version to be bumped.

Please double check userspace/engine/falco_engine_version.h file. See versioning for FALCO_ENGINE_VERSION.

/hold

incertum · 2023-08-23T23:07:20Z

falco.yaml

@@ -613,7 +613,7 @@ syscall_event_drops:
  max_burst: 1
  simulate_drops: false

-# [Experimental] `metrics`
+# [Stable] `metrics`


@FedeDP @leogr WDYT? Other than that one odd behavior where the timer did not work when using http output (which Federico fixed) I have not heard of more issues when using the metrics feature, have you? It is working well for me in production.

Proposing to bump to Stable, which does not mean that there may be no additional bugs, it rather signals we are supporting metrics now as stable feature going forward.

I think this is good 2 go! Agree with you.
@leogr more thoughts?

Agree 👍

cc @falcosecurity/falco-maintainers for visibility

incertum · 2023-08-23T23:08:57Z

/milestone 0.36.0

leogr

SGTM. I am just waiting for more opinions before giving my approval.

leogr · 2023-08-24T12:53:25Z

falco.yaml

@@ -613,7 +613,7 @@ syscall_event_drops:
  max_burst: 1
  simulate_drops: false

-# [Experimental] `metrics`
+# [Stable] `metrics`


Agree 👍

cc @falcosecurity/falco-maintainers for visibility

leogr · 2023-08-24T12:54:32Z

userspace/falco/app/actions/load_config.cpp

-			}
-		}
-	}
+	// Keep for future use cases.


Andreagit97

Just one minor cleanup otherwise LGTM!
Talking with Fede, he correctly pointed out that removing -s would break all current Falco settings that are running with this flag enabled by default, but in the end, we depracted it in Falco 0.35 so it should be fine

Andreagit97 · 2023-08-24T13:59:56Z

userspace/falco/app/options.cpp

@@ -219,8 +219,6 @@ void options::define(cxxopts::Options& opts)
 		("p,print",                       "Add additional information to each falco notification's output.\nWith -pc or -pcontainer will use a container-friendly format.\nWith -pk or -pkubernetes will use a kubernetes-friendly format.\nAdditionally, specifying -pc/-pk will change the interpretation of %container.info in rule output fields.", cxxopts::value(print_additional), "<output_format>")
 		("P,pidfile",                     "When run as a daemon, write pid to specified file", cxxopts::value(pidfilename)->default_value("/var/run/falco.pid"), "<pid_file>")
 		("r",                             "Rules file/directory (defaults to value set in configuration file, or /etc/falco_rules.yaml). This option can be passed multiple times to read from multiple files/directories.", cxxopts::value<std::vector<std::string>>(), "<rules_file>")
-		("s",                             "If specified, append statistics related to Falco's reading/processing of events to this file (only useful in live mode).", cxxopts::value(stats_output_file), "<stats_file>")


we need to remove also stats_output_file and stats_interval variables

Thank you! Also rebased since we adjusted the build setup a bit.

…ics configs in falco.yaml Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

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

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

Andreagit97

/approve
Thanks

poiana · 2023-08-25T08:21:11Z

LGTM label has been added.

Git tree hash: 98c70c66d4c2b0a9c065f10703b08f56f5d4767b

FedeDP

/approve

FedeDP · 2023-08-25T08:26:07Z

Per #2739 (comment), do we need an engine bump?

Andreagit97 · 2023-08-25T08:28:11Z

/hold

Andreagit97 · 2023-08-25T08:35:07Z

Per #2739 (comment), do we need an engine bump?

uhm the message was printed because this PR touches a file under /userspace/engine/* but not sure we need an engine version bump in the end the logic is almost the same we removed a fallback check 🤔 cc @leogr

Andreagit97 · 2023-08-25T10:42:16Z

userspace/falco/app/actions/process_events.cpp

@@ -435,7 +435,6 @@ static falco::app::run_result init_stats_writer(
 		falco_logger::log(LOG_WARNING, "Metrics are enabled with no output configured, no snapshot will be collected");
 	}



I cannot comment on the right line but above (https://github.com/falcosecurity/falco/pull/2739/files#diff-3a3a5ebb7d4bf516a4c732cadcd83feabe05a5cc76a9ca9e1f55802a94b3c4a7R428), there is a check on the prometheus stats format that say

falco_logger::log(LOG_WARNING, "Metrics interval was passed as numeric value without Prometheus time unit, this option will be deprecated in the future");

Probably we want to turn it into an falco::app::run_result::fatal , WDYT?
It could be a future cleanup if we want merge this :)

Andreagit97 · 2023-08-25T10:45:21Z

userspace/falco/app/actions/process_events.cpp

@@ -435,7 +435,6 @@ static falco::app::run_result init_stats_writer(
 		falco_logger::log(LOG_WARNING, "Metrics are enabled with no output configured, no snapshot will be collected");


Moreover, this is a misconfiguration of the tool an IMO we should fail, WDYT?
Not related to this PR but just a consideration

leogr · 2023-08-25T13:19:31Z

Per #2739 (comment), do we need an engine bump?

uhm the message was printed because this PR touches a file under /userspace/engine/* but not sure we need an engine version bump in the end the logic is almost the same we removed a fallback check 🤔 cc @leogr

Correct, it was a false positive. We can merge this without bumping the engine version.
cc @jasondellaluce

leogr

/hold cancel

poiana · 2023-08-25T13:19:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Andreagit97, FedeDP, 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,FedeDP,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 approved labels Aug 23, 2023

poiana requested review from Kaizhe and leogr August 23, 2023 23:04

poiana added the size/L label Aug 23, 2023

poiana added the do-not-merge/hold label Aug 23, 2023

incertum commented Aug 23, 2023

View reviewed changes

poiana added this to the 0.36.0 milestone Aug 23, 2023

leogr reviewed Aug 24, 2023

View reviewed changes

Andreagit97 reviewed Aug 24, 2023

View reviewed changes

incertum and others added 4 commits August 24, 2023 16:48

feat(userspace): deprecate stats command args option in favor of metr…

c46405f

…ics configs in falco.yaml Signed-off-by: Melissa Kilby <melissa.kilby.oss@gmail.com>

cleanup(userspace): update parse_prometheus_interval

a575464

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

cleanup(config): assign Stable to metrics config

a0c0a0e

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

cleanup: remove some unused variables

5761c58

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-flags branch from d683bfa to 5761c58 Compare August 24, 2023 17:26

Andreagit97 approved these changes Aug 25, 2023

View reviewed changes

poiana assigned Andreagit97 Aug 25, 2023

poiana added the lgtm label Aug 25, 2023

FedeDP approved these changes Aug 25, 2023

View reviewed changes

poiana assigned FedeDP Aug 25, 2023

Andreagit97 reviewed Aug 25, 2023

View reviewed changes

leogr approved these changes Aug 25, 2023

View reviewed changes

poiana removed the do-not-merge/hold label Aug 25, 2023

poiana assigned leogr Aug 25, 2023

poiana merged commit b66bf2c into falcosecurity:master Aug 25, 2023

Andreagit97 mentioned this pull request Aug 29, 2023

cleanup: turn some warnings into errors #2744

Merged

LucaGuerra mentioned this pull request Sep 7, 2023

[TRACKING] Breaking changes in Falco 0.37.0 #2763

Closed

6 tasks

Andreagit97 changed the title ~~feat(userspace): deprecate stats command args option in favor of metrics configs in falco.yaml~~ feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml Sep 26, 2023

Andreagit97 mentioned this pull request Sep 26, 2023

[TRACKING] Breaking changes in Falco 0.36.0 #2788

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml #2739

feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml #2739

incertum commented Aug 23, 2023 •

edited by leogr

Loading

github-actions bot commented Aug 23, 2023

incertum Aug 23, 2023

FedeDP Aug 24, 2023

leogr Aug 24, 2023

incertum commented Aug 23, 2023

leogr left a comment

leogr Aug 24, 2023

leogr Aug 24, 2023

Andreagit97 left a comment

Andreagit97 Aug 24, 2023

incertum Aug 24, 2023

Andreagit97 left a comment

poiana commented Aug 25, 2023

FedeDP left a comment

FedeDP commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023

Andreagit97 Aug 25, 2023

Andreagit97 Aug 25, 2023

leogr commented Aug 25, 2023

leogr left a comment

poiana commented Aug 25, 2023

		@@ -435,7 +435,6 @@ static falco::app::run_result init_stats_writer(
		falco_logger::log(LOG_WARNING, "Metrics are enabled with no output configured, no snapshot will be collected");
		}

feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml #2739

feat(userspace)!: deprecate stats command args option in favor of metrics configs in falco.yaml #2739

Conversation

incertum commented Aug 23, 2023 • edited by leogr Loading

github-actions bot commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

incertum commented Aug 23, 2023

leogr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Aug 25, 2023

FedeDP left a comment

Choose a reason for hiding this comment

FedeDP commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023

Andreagit97 commented Aug 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leogr commented Aug 25, 2023

leogr left a comment

Choose a reason for hiding this comment

poiana commented Aug 25, 2023

incertum commented Aug 23, 2023 •

edited by leogr

Loading