cleanup(sinsp/metrics): assorted cleanups for robustness #1950

incertum · 2024-07-05T18:02:10Z

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

Any specific area of the project related to this PR?

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

/area API-version

/area build

/area CI

/area driver-kmod

/area driver-bpf

/area driver-modern-bpf

/area libscap-engine-bpf

/area libscap-engine-gvisor

/area libscap-engine-kmod

/area libscap-engine-modern-bpf

/area libscap-engine-nodriver

/area libscap-engine-noop

/area libscap-engine-source-plugin

/area libscap-engine-savefile

/area libscap

/area libpman

/area libsinsp

/area tests

/area proposals

Does this PR require a change in the driver versions?

/version driver-API-version-major

/version driver-API-version-minor

/version driver-API-version-patch

/version driver-SCHEMA-version-major

/version driver-SCHEMA-version-minor

/version driver-SCHEMA-version-patch

What this PR does / why we need it:

remove raw_name label, see Metrics don't follow Prometheus best practices falco#3271 (comment)
move sanitization methods to libs, see [TRACKING] metrics framework future refactors / cleanups falco#3194 (comment)

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Signed-off-by: Melissa Kilby <[email protected]>

incertum · 2024-07-05T18:02:24Z

CC @Issif @sgaist @FedeDP

incertum · 2024-07-05T18:02:32Z

/milestone 0.18.0

github-actions · 2024-07-05T18:09:31Z

Perf diff from master - unit tests

    11.92%     -1.18%  [.] sinsp_parser::reset
     1.00%     +0.74%  [.] sinsp::fetch_next_event
     7.16%     +0.74%  [.] sinsp::next
     4.64%     -0.69%  [.] next
     4.10%     -0.67%  [.] gzfile_read
     3.90%     -0.64%  [.] sinsp_evt::load_params
     2.00%     -0.63%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     0.74%     +0.60%  [.] sinsp_parser::event_cleanup
     2.15%     +0.57%  [.] scap_event_decode_params
     0.70%     +0.55%  [.] sinsp_parser::parse_context_switch

Perf diff from master - scap file

    10.08%     -3.82%  [.] sinsp_filter_check::tostring
     3.36%     +3.63%  [.] sinsp_filter_check_event::extract_single
     3.34%     +3.20%  [.] sinsp_parser::process_event
     3.21%     +2.51%  [.] sinsp_filter_check::get_field_info
     6.65%     -2.01%  [.] sinsp_utils::ts_to_string
     3.27%     +1.81%  [.] sinsp_filter_check::apply_transformers
     3.31%     +1.38%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     6.72%     -0.73%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
    13.36%     -0.53%  [.] sinsp_filter_check::extract_nocache
    13.40%     +0.49%  [.] sinsp_evt_formatter::tostring_withformat

Heap diff from master - unit tests

total runtime: 0.10s.
calls to allocation functions: -5935 (-60561/s)
temporary memory allocations: -465 (-4744/s)
peak heap memory consumption: -2.43K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.01s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -3 (-214/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

sgaist

That's a good cleanup !

sgaist · 2024-07-08T08:33:48Z

userspace/libsinsp/metrics_collector.cpp

+	if (!sanitized_name.empty() && !(std::isalnum(sanitized_name.front()) || sanitized_name.front() == '_'))
+	{
+		sanitized_name = "_" + sanitized_name;
+	}
+	if (!sanitized_name.empty() && sanitized_name.back() == '_')
+	{
+		sanitized_name.pop_back();
+	}


I am not sure to follow the logic:
if not empty, an underscore is prepended if the first char is not alpha numeric or if it is already an underscore.
The former should not happen based on the RegEx and for the latter, the documentation state that double underscore starting names are reserved for internal use.

Also, if the metric name is empty, then nothing happen, shouldn't it scream about it ?

Misread the regex and thought for the metric name also numbers are allowed in front. Plus yes the !sanitized_name.empty() was an error here. Re-pushed to 100% strictly stay with the prometheus mandates.

Right now we would replace and empty metrics name with _. We chatted a bit over slack and are unsure how to best handle it. Logs could be too noisy and since metrics are not that critical, we certainly don't want to error out or anything. Maybe simply not adding the metric is the right solution.

sgaist · 2024-07-08T08:49:29Z

userspace/libsinsp/metrics_collector.cpp

+std::string prometheus_sanitize_metric_name(const std::string& name)
+{
+	// https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels
+	static const RE2 invalid_chars("[^a-zA-Z0-9_]");
+	std::string sanitized_name = name;
+	RE2::GlobalReplace(&sanitized_name, invalid_chars, "_");
+	RE2::GlobalReplace(&sanitized_name, "_+", "_");
+	if (!sanitized_name.empty() && !(std::isalnum(sanitized_name.front()) || sanitized_name.front() == '_'))
+	{
+		sanitized_name = "_" + sanitized_name;
+	}
+	if (!sanitized_name.empty() && sanitized_name.back() == '_')
+	{
+		sanitized_name.pop_back();
+	}
+	return sanitized_name;
+}
+
+std::string prometheus_sanitize_label_name(const std::string& name)
+	{
+	// https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels
+	static const RE2 invalid_chars("[^a-zA-Z0-9_]");
+	std::string sanitized_label = name;
+	RE2::GlobalReplace(&sanitized_label, invalid_chars, "_");
+	RE2::GlobalReplace(&sanitized_label, "_+", "_");
+
+	// Ensure the label starts with a letter or underscore (if empty after sanitizing, set to "_")
+	if (sanitized_label.empty() || (!std::isalpha(sanitized_label.front()) && sanitized_label.front() != '_'))
+	{
+		sanitized_label = "_" + sanitized_label;
+	}
+
+	return sanitized_label;
+}


Unrelated to the logic question, I would factor out the common part to avoid code duplication since both are using exactly the same cleanup starting phase.

Suggested change

std::string prometheus_sanitize_metric_name(const std::string& name)

{

// https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

static const RE2 invalid_chars("[^a-zA-Z0-9_]");

std::string sanitized_name = name;

RE2::GlobalReplace(&sanitized_name, invalid_chars, "_");

RE2::GlobalReplace(&sanitized_name, "_+", "_");

if (!sanitized_name.empty() && !(std::isalnum(sanitized_name.front()) || sanitized_name.front() == '_'))

{

sanitized_name = "_" + sanitized_name;

}

if (!sanitized_name.empty() && sanitized_name.back() == '_')

{

sanitized_name.pop_back();

}

return sanitized_name;

}

std::string prometheus_sanitize_label_name(const std::string& name)

{

// https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

static const RE2 invalid_chars("[^a-zA-Z0-9_]");

std::string sanitized_label = name;

RE2::GlobalReplace(&sanitized_label, invalid_chars, "_");

RE2::GlobalReplace(&sanitized_label, "_+", "_");

// Ensure the label starts with a letter or underscore (if empty after sanitizing, set to "_")

if (sanitized_label.empty() || (!std::isalpha(sanitized_label.front()) && sanitized_label.front() != '_'))

{

sanitized_label = "_" + sanitized_label;

}

return sanitized_label;

}

std::string _sanitize_name(const std::string& name)

{

// https://prometheus.io/docs/concepts/data_model/#metric-names-and-labels

static const RE2 invalid_chars("[^a-zA-Z0-9_]");

std::string sanitized_name = name;

// RE2::Extract(name, invalid_chars, "_", &sanitized_name);

RE2::GlobalReplace(&sanitized_name, invalid_chars, "_");

RE2::GlobalReplace(&sanitized_name, "_+", "_");

return sanitized_name;

}

std::string prometheus_sanitize_metric_name(const std::string& name)

{

std::string sanitized_name = _sanitize_name(name);

if (!sanitized_name.empty())

{

if (!(std::isalnum(sanitized_name.front()) || sanitized_name.front() == '_'))

{

sanitized_name.insert(0, "_");

}

if (sanitized_name.back() == '_')

{

sanitized_name.pop_back();

}

}

return sanitized_name;

}

std::string prometheus_sanitize_label_name(const std::string& name)

{

std::string sanitized_label = _sanitize_name(name);

// Ensure the label starts with a letter or underscore (if empty after sanitizing, set to "_")

if (sanitized_label.empty() || (!std::isalpha(sanitized_label.front()) && sanitized_label.front() != '_'))

{

sanitized_label.insert(0, "_");

}

return sanitized_label;

}

Added an alternative solution as second co-authored commit, let me know.

@sgaist kind bump, ty!

sgaist · 2024-07-08T08:50:41Z

userspace/libsinsp/metrics_collector.cpp

 	{
-		prometheus_text += "," + key + "=\"" + value + "\"" ;
+		prometheus_text += " "; // white space at the end important!


Suggested change

prometheus_text += " "; // white space at the end important!

prometheus_text += " "; // the white space at the end is important!

sgaist · 2024-07-08T08:51:09Z

userspace/libsinsp/metrics_collector.cpp

+			}
+			prometheus_text += prometheus_sanitize_label_name(key) + "=\"" + value + "\"";
+		}
+		prometheus_text += "} "; // white space at the end important!


Suggested change

prometheus_text += "} "; // white space at the end important!

prometheus_text += "} "; // the white space at the end is important!

poiana · 2024-07-08T09:14:59Z

@sgaist: changing LGTM is restricted to collaborators

In response to this:

That's a good cleanup !

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…ation methods Signed-off-by: Melissa Kilby <[email protected]>

Co-authored-by: Samuel Gaist <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

github-actions · 2024-07-08T21:49:23Z

Perf diff from master - unit tests

    11.93%     -2.57%  [.] sinsp_parser::reset
     4.20%     +1.28%  [.] sinsp_parser::process_event
     4.81%     +1.20%  [.] sinsp_thread_manager::find_thread
     4.64%     -1.20%  [.] next
     7.16%     -0.75%  [.] sinsp::next
     0.85%     -0.61%  [.] scap_next
     1.00%     +0.51%  [.] sinsp::fetch_next_event
     1.25%     -0.49%  [.] sinsp_fdtable::find_ref
     0.74%     +0.47%  [.] sinsp_parser::event_cleanup
     1.06%     -0.46%  [.] sinsp_threadinfo::~sinsp_threadinfo

Perf diff from master - scap file

    13.88%     -3.24%  [.] sinsp_evt_formatter::tostring_withformat
    13.84%     -3.23%  [.] sinsp_filter_check::extract_nocache
     3.43%     +2.48%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char const*>
     6.97%     +2.40%  [.] std::_Hashtable<long, std::pair<long const, std::shared_ptr<sinsp_threadinfo> >, std::allocator<std::pair<long const, std::shared_ptr<sinsp_threadinfo> > >, std::__detail::_Select1st, std::equal_to<long>, std::hash<long>, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<false, false, true> >::_M_find_before_node
     3.49%     +2.29%  [.] sinsp_filter_check_event::extract_single
     3.26%     +2.14%  [.] std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_construct<char*>
     3.48%     +2.08%  [.] libsinsp::runc::match_container_id
     3.48%     +2.00%  [.] 0x00000000000a7310
     6.95%     -1.62%  [.] sinsp::next
     6.89%     -1.61%  [.] sinsp_parser::reset

Heap diff from master - unit tests

total runtime: 0.02s.
calls to allocation functions: -10684 (-534200/s)
temporary memory allocations: -729 (-36450/s)
peak heap memory consumption: 1.40K
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Heap diff from master - scap file

total runtime: 0.02s.
calls to allocation functions: 0 (0/s)
temporary memory allocations: -3 (-130/s)
peak heap memory consumption: 0B
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

poiana · 2024-08-08T18:17:35Z

LGTM label has been added.

Git tree hash: 6d68da1fdf4ccef12e2abfe2ead351d23db0bb43

FedeDP

/approve

Andreagit97

/approve

poiana · 2024-08-20T16:20:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

cleanup(sinsp/metrics): remove raw_name as label

725b633

Signed-off-by: Melissa Kilby <[email protected]>

poiana added this to the 0.18.0 milestone Jul 5, 2024

poiana added release-note-none dco-signoff: yes kind/cleanup labels Jul 5, 2024

poiana requested review from Andreagit97 and hbrueckner July 5, 2024 18:05

poiana added approved area/libsinsp size/L labels Jul 5, 2024

sgaist suggested changes Jul 8, 2024

View reviewed changes

incertum and others added 2 commits July 8, 2024 21:22

cleanup(sinsp/metrics): add prometheus metric and label names sanitiz…

3dbfd82

…ation methods Signed-off-by: Melissa Kilby <[email protected]>

cleanup(metrics/prometheus): collapse sanitization method

3bd4dc8

Co-authored-by: Samuel Gaist <[email protected]> Signed-off-by: Melissa Kilby <[email protected]>

incertum force-pushed the metrics-cleanup branch from e8cae6a to 3bd4dc8 Compare July 8, 2024 21:45

sgaist approved these changes Aug 8, 2024

View reviewed changes

poiana assigned sgaist Aug 8, 2024

poiana added the lgtm label Aug 8, 2024

FedeDP approved these changes Aug 20, 2024

View reviewed changes

poiana assigned FedeDP Aug 20, 2024

Andreagit97 approved these changes Aug 20, 2024

View reviewed changes

poiana assigned Andreagit97 Aug 20, 2024

poiana merged commit 1dc7459 into falcosecurity:master Aug 20, 2024
42 of 43 checks passed

incertum mentioned this pull request Aug 31, 2024

[TRACKING] metrics framework future refactors / cleanups falcosecurity/falco#3194

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup(sinsp/metrics): assorted cleanups for robustness #1950

cleanup(sinsp/metrics): assorted cleanups for robustness #1950

incertum commented Jul 5, 2024

incertum commented Jul 5, 2024

incertum commented Jul 5, 2024

github-actions bot commented Jul 5, 2024

sgaist left a comment

sgaist Jul 8, 2024

incertum Jul 8, 2024

sgaist Jul 8, 2024

incertum Jul 8, 2024

incertum Aug 8, 2024

sgaist Jul 8, 2024

sgaist Jul 8, 2024

poiana commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

poiana commented Aug 8, 2024

FedeDP left a comment

Andreagit97 left a comment

poiana commented Aug 20, 2024

	prometheus_text += " "; // white space at the end important!
	prometheus_text += " "; // the white space at the end is important!

	prometheus_text += "} "; // white space at the end important!
	prometheus_text += "} "; // the white space at the end is important!

cleanup(sinsp/metrics): assorted cleanups for robustness #1950

cleanup(sinsp/metrics): assorted cleanups for robustness #1950

Conversation

incertum commented Jul 5, 2024

incertum commented Jul 5, 2024

incertum commented Jul 5, 2024

github-actions bot commented Jul 5, 2024

Perf diff from master - unit tests

Perf diff from master - scap file

Heap diff from master - unit tests

Heap diff from master - scap file

sgaist left a comment

Choose a reason for hiding this comment

sgaist Jul 8, 2024

Choose a reason for hiding this comment

incertum Jul 8, 2024

Choose a reason for hiding this comment

sgaist Jul 8, 2024

Choose a reason for hiding this comment

incertum Jul 8, 2024

Choose a reason for hiding this comment

incertum Aug 8, 2024

Choose a reason for hiding this comment

sgaist Jul 8, 2024

Choose a reason for hiding this comment

sgaist Jul 8, 2024

Choose a reason for hiding this comment

poiana commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

Perf diff from master - unit tests

Perf diff from master - scap file

Heap diff from master - unit tests

Heap diff from master - scap file

poiana commented Aug 8, 2024

FedeDP left a comment

Choose a reason for hiding this comment

Andreagit97 left a comment

Choose a reason for hiding this comment

poiana commented Aug 20, 2024