filterx: `update_metric()` #220

alltilla · 2024-07-22T06:58:52Z

Example:

update_metric("filterx_metric", labels={"msg": $MSG, "foo": "foovalue"}, level=1, increment=$INCREMENT)

alltilla · 2024-07-26T10:08:06Z

Ready for review.

lib/filterx/filterx-metrics-labels.c

tests/light/functional_tests/filterx/test_filterx_update_metric.py

lib/filterx/filterx-metrics-labels.c

alltilla · 2024-08-03T08:10:35Z

rebased to main

bazsi · 2024-08-04T13:22:46Z

lib/filterx/filterx-metrics-labels.c

+gboolean
+filterx_metrics_labels_format(FilterXMetricsLabels *self, StatsClusterLabel **labels, gsize *len)
+{
+  metrics_cache_reset_labels(metrics_tls_cache());


is there a point in saving the metrics_tls_cache() value and not do it 3x?

Thanks, fixed in a new commit.

bazsi · 2024-08-04T13:30:57Z

lib/filterx/filterx-metrics-labels.c

+      return FALSE;
+    }
+
+  StatsClusterLabel *label = metrics_cache_alloc_label(metrics_tls_cache());


actually it might even make sense to cache the metrics_tls_cache() across the entire iteration. Getting the value of a TLS variable is not free, especially when wrapped into an external function, like in this case.

Just to clarify, I meant that it would make sense to pass the value around to callbacks that iterate possibly multiple times per message to avoid the TLS overhead.

I did not think about the overhead. Thanks, fixed this in a new commit.

lib/filterx/filterx-metrics-labels.c

bazsi · 2024-08-04T13:47:06Z

lib/filterx/filterx-metrics.c

+      return FALSE;
+    }
+
+  self->key.str = g_strdup(filterx_string_get_value(key_obj, NULL));


probably filterx_object_extract_string() would make sense here (depends on PR #229)

maybe the plan is to merge this first, in that case of course #229 should address this.

In this specific scenario, we don't need to call the extract function, as we check before that it is a literal, and there are no literal message values.

lib/filterx/filterx-metrics.c

bazsi · 2024-08-04T13:50:35Z

lib/filterx/filterx-metrics.c

+    }
+
+  GString *name_buffer = scratch_buffers_alloc();
+  g_string_append(name_buffer, name);


I guess you were trying to avoid a g_strdup() here. in that case, it makes sense to use g_string_append_len(), otherwise you'd be doing an strlen() as a part of the copy, while the length is already available.

Thanks, fixed and squashed to the original commit.

bazsi · 2024-08-04T13:52:25Z

modules/metrics-probe/filterx/func-update-metric.c

+      return TRUE;
+    }
+
+  FilterXObject *increment_obj = filterx_expr_eval_typed(self->increment.expr);


extract_integer.

would depend on #229, so depends on merge order.

bazsi · 2024-08-04T13:54:44Z

modules/metrics-probe/filterx/func-update-metric.c

+exit:
+  if (!success)
+    {
+      msg_error("FilterX: Failed to process update_metric()", filterx_format_last_error());


I think it is a bad idea to start generating a message in these cases. I think it'd be much better to add an "error" counter along with all update_metric() counters and increment that every time something fails.

With that we could even add limits on how many counters we would be adding in case there's a cardinality explosion.

it does not need to be done now, just an idea.

I agree, changed the log level to debug in a new commit. We can introduce a counter later.

bazsi · 2024-08-04T13:57:12Z

lib/filterx/filterx-metrics.c

+   */
+  if (!self->is_optimized)
+    {
+      if (!_optimize(self))


I think this needs to be protected by a lock, quite probably we can reuse the starts_lock() in this case.

Nice catch! Fixed in a new commit.

alltilla · 2024-08-06T07:44:29Z

Thanks for the thorough review @bazsi.

I have fixed all the comments, most of them went to new commits to make it easier to review, starting with the commit filterx/metrics-labels: reduce the number of tls cache acquirals

The order of this PR and #229 is undecided, I will update the one which gets merged later.

Signed-off-by: Attila Szakacs <[email protected]>

This class is responsible for formatting a `FilterXExpr` to an array of `StatsClusterLabel`s. The expression must be evaluate to a `Dict[str, Any]`. The formatting is cached wherever it is possible. The implementation uses TLS metrics cache, as `FilterXExpr`s must be stateless currently, but if the need arises, the code can be easily changed to use an arbitrary metrics cache. Signed-off-by: Attila Szakacs <[email protected]>

Signed-off-by: Attila Szakacs <[email protected]>

This class is responsible for managing `StatsCounterItem`s based on `key` and `labels` `FilterXExpr`s. `key` must evaluate to a `str`. `labels` must evaluate to a `Dict[str, Any]` is handled by `FilterXMetricsLabels`. The implementation uses TLS metrics cache, as `FilterXExpr`s must be stateless currently, but if the need arises, the code can be easily changed to use an arbitrary metrics cache Signed-off-by: Attila Szakacs <[email protected]>

Signed-off-by: Attila Szakacs <[email protected]>

We need to delay the optimization to the first get() call as we don't have stats options when FilterXExprs are being created. Signed-off-by: Attila Szakacs <[email protected]>

Signed-off-by: Attila Szakacs <[email protected]>

It is not used anywhere. Signed-off-by: Attila Szakacs <[email protected]>

Signed-off-by: Attila Szakacs <[email protected]>

This could easily flood the logs if something is incorrectly configured. Signed-off-by: Attila Szakacs <[email protected]>

If optimization did not succeed, probably it won't in the future, so just skip it. Signed-off-by: Attila Szakacs <[email protected]>

Signed-off-by: Attila Szakacs <[email protected]>

axoflow/axosyslog#220

alltilla force-pushed the filterx-metrics-probe branch 4 times, most recently from d3d37cc to db74448 Compare July 25, 2024 10:53

alltilla changed the title ~~filterx: metrics_probe()~~ filterx: update_metric() Jul 25, 2024

alltilla force-pushed the filterx-metrics-probe branch 4 times, most recently from 27789cc to 5a8a604 Compare July 26, 2024 10:07

alltilla requested review from bshifter and MrAnno July 26, 2024 10:08

OverOrion reviewed Jul 30, 2024

View reviewed changes

MrAnno reviewed Jul 31, 2024

View reviewed changes

lib/filterx/filterx-metrics-labels.c Show resolved Hide resolved

alltilla force-pushed the filterx-metrics-probe branch from 5a8a604 to 496b2a0 Compare August 2, 2024 11:04

OverOrion previously approved these changes Aug 2, 2024

View reviewed changes

alltilla force-pushed the filterx-metrics-probe branch from 496b2a0 to 595a7d2 Compare August 3, 2024 08:02

bazsi reviewed Aug 4, 2024

View reviewed changes

lib/filterx/filterx-metrics-labels.c Show resolved Hide resolved

bazsi reviewed Aug 4, 2024

View reviewed changes

lib/filterx/filterx-metrics.c Show resolved Hide resolved

bazsi reviewed Aug 4, 2024

View reviewed changes

bazsi previously approved these changes Aug 4, 2024

View reviewed changes

alltilla dismissed stale reviews from bazsi and OverOrion via 16a8251 August 6, 2024 07:38

alltilla force-pushed the filterx-metrics-probe branch from 595a7d2 to 16a8251 Compare August 6, 2024 07:38

alltilla added 22 commits August 6, 2024 10:11

filterx: support dict and list generator in function named params

9ea9876

Signed-off-by: Attila Szakacs <[email protected]>

filterx: add function plugin helper templates

689dc41

Signed-off-by: Attila Szakacs <[email protected]>

metrics: include <string.h> for strcmp

c1a4ddc

Signed-off-by: Attila Szakacs <[email protected]>

metrics-cache: add sort_labels()

43420f1

Signed-off-by: Attila Szakacs <[email protected]>

filterx/expr-function: add the rest of the primitive named getters

e28ced3

Signed-off-by: Attila Szakacs <[email protected]>

filterx: add type checkers for generators

79ba0ed

Signed-off-by: Attila Szakacs <[email protected]>

filterx/expr-literal-generator: add dict foreach()

218a15a

Signed-off-by: Attila Szakacs <[email protected]>

filterx: add metrics_labels UT

7f8dd2a

Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe/filterx: add build skeleton

44e5c23

Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe/filterx: implement update_metric()

daa7a49

Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe: delay optimization in filterx update_metric()

3ac024a

We need to delay the optimization to the first get() call as we don't have stats options when FilterXExprs are being created. Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe: extract some UT related functions

8393bb4

Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe: add filterx update_metric() UT

aaf1256

Signed-off-by: Attila Szakacs <[email protected]>

metrics-probe: add filterx update_metric() E2E tests

ce15160

Signed-off-by: Attila Szakacs <[email protected]>

filterx/metrics-labels: reduce the number of tls cache acquirals

318a6b9

Signed-off-by: Attila Szakacs <[email protected]>

metrics-template: do not expose metrics_template_build_sck()

1d8ae45

It is not used anywhere. Signed-off-by: Attila Szakacs <[email protected]>

metrics-template: reduce the number of tls cache acquirals

10e4d9b

Signed-off-by: Attila Szakacs <[email protected]>

filterx/func-update-metric: reduce error log level to debug

a3a3e89

This could easily flood the logs if something is incorrectly configured. Signed-off-by: Attila Szakacs <[email protected]>

filterx/filterx-metrics: optimize once

78fd0e0

If optimization did not succeed, probably it won't in the future, so just skip it. Signed-off-by: Attila Szakacs <[email protected]>

filterx/filterx-metrics: fix concurrency in _optimize()

ab668f7

Signed-off-by: Attila Szakacs <[email protected]>

alltilla force-pushed the filterx-metrics-probe branch from 16a8251 to ab668f7 Compare August 6, 2024 11:12

bazsi approved these changes Aug 6, 2024

View reviewed changes

alltilla merged commit 8581cd8 into axoflow:main Aug 6, 2024
22 checks passed

fekete-robert pushed a commit to axoflow/axosyslog-core-docs that referenced this pull request Nov 11, 2024

[4.9][filterx] Adds update_metric

d941258

axoflow/axosyslog#220

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filterx: `update_metric()` #220

filterx: `update_metric()` #220

alltilla commented Jul 22, 2024 •

edited

Loading

alltilla commented Jul 26, 2024

alltilla commented Aug 3, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

bazsi Aug 4, 2024

alltilla Aug 6, 2024

alltilla commented Aug 6, 2024

filterx: update_metric() #220

filterx: update_metric() #220

Conversation

alltilla commented Jul 22, 2024 • edited Loading

alltilla commented Jul 26, 2024

alltilla commented Aug 3, 2024

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

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

Choose a reason for hiding this comment

alltilla commented Aug 6, 2024

filterx: `update_metric()` #220

filterx: `update_metric()` #220

alltilla commented Jul 22, 2024 •

edited

Loading