Metrics best practice review (3 issues found)

[leogr]

Describe the bug

1. falcosecurity_falco_sha256_config_files_info : file ext is missing in file_name, it should be added
2. falcosecurity_falco_host_ifinfo_json_info : encoding a json should be against the best practices; possibile solutions: break down this metric into individual labels or remove it if not necessary
3. tags label contains multiple values, we may split them like tag_t1059="true", tag_container="true", tag_maturity_stable="true", tag_mitre_execution="true", tag_shell="true"
   • the cardinality should stay under control because each rule has always the same set of labels and won't change during runtime (basically the unique combos of tags are intrinsically capped by the ruleset and can't grow - unless you load more rules)

How to reproduce it

I used https://download.falco.org/packages/bin-dev/x86_64/falco-0.39.0-rc2-x86_64.tar.gz and run:

falco -o metrics.enabled=true -o webserver.prometheus_metrics_enabled=true

Expected behaviour

Metrics align to best practices

Environment

falco --version
Fri Sep 20 11:51:38 2024: Falco version: 0.39.0-rc2 (x86_64)
Fri Sep 20 11:51:38 2024: Falco initialized with configuration files:
Fri Sep 20 11:51:38 2024:    /etc/falco/falco.yaml | schema validation: ok
Fri Sep 20 11:51:38 2024: System info: Linux version 6.10.10-arch1-1 (linux@archlinux) (gcc (GCC) 14.2.1 20240910, GNU ld (GNU Binutils) 2.43.0) #1 SMP PREEMPT_DYNAMIC Thu, 12 Sep 2024 17:21:02 +0000
Falco version: 0.39.0-rc2
Libs version:  0.18.0
Plugin API:    3.7.0
Engine:        0.43.0
Driver:
  API version:    8.0.0
  Schema version: 2.0.0
  Default driver: 7.3.0+driver

Additional context

Tentatively for /milestone 0.39.0

cc @falcosecurity/falco-maintainers @alacuku @Issif

[Issif]

I vote for the removing the metric with the json, it breaks prometheus scraping

[FedeDP]

@incertum FYI

[FedeDP]

falcosecurity_falco_sha256_config_files_info : file ext is missing in file_name, it should be added

Weird, the code is correct: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L111 EDIT: oh you are talking about the extension, sorry! Then, it seems like it was desired, since we are using .stem() method (https://en.cppreference.com/w/cpp/filesystem/path/stem)

For tags, i assume you are talking about rule tags; if yes, then the change needs to be done here: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L237

Re host_ifinfo, the change is to be done here: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L146 Since host ifinfo were added recently: https://github.com/falcosecurity/falco/pull/3253, i wouldn't remove them unless @incertum is ok with that.

[leogr]

Weird, the code is correct: https://github.com/falcosecurity/falco/blob/master/userspace/falco/falco_metrics.cpp#L111 EDIT: oh you are talking about the extension, sorry! Then, it seems like it was desired, since we are using .stem() method (https://en.cppreference.com/w/cpp/filesystem/path/stem)

I'm wondering why it should be considered:thinking: I can't find a compelling reason, but I may be missing the point.

[ekoops]

Regarding falcosecurity_falco_host_ifinfo_json_info, the interfaces/addresses number could be high in some environment: if we split them in some meaningful way, the number of dimensions can become high. Moreover, the interfaces/addresses list could be highly mutable, but at the moment, the backing list is not refreshed after its initialization (there is a method sinsp::refresh_ifaddr_list() but it doesn't seem to be called anywhere). Given these two points, maybe it is better to remove it.

[leogr]

Regarding falcosecurity_falco_host_ifinfo_json_info, the interfaces/addresses number could be high in some environment: if we split them in some meaningful way, the number of dimensions can become high. Moreover, the interfaces/addresses list could be highly mutable, but at the moment, the backing list is not refreshed after its initialization (there is a method sinsp::refresh_ifaddr_list() but it doesn't seem to be called anywhere). Given these two points, maybe it is better to remove it.

I agree with removing it for now and target this for 0.40