Limit dynamic counters #1743

lbudai · 2017-10-27T19:47:40Z

Add config option for limiting dynamic counters

Users can limit the number of registered dynamic counters via the stats_max_dynamics global option.
This is not a limit for the number of counters but for the number of clusters which means that users
can limit the number of different dynamic value occurrences (as each
cluster has a pre-allocated array of counters).
Note that after reload, when users set a lower limit,
they have to wait for stat-freq-timer being expired to remove the
previously allocated dynamic clusters.
Dynamic values are
- source host (from stats level 2)
- sender host (host from) (from stats level 3)
- program (from stats level 3)
By default this change is backward compatible: if someone not uses this feature, number of dynamic counters won't be limited.
It is possible to disable dynamic counters by setting the limit to 0.

The following example contains 6 different dynamic values (a sender, a host and four different programs):

    src.sender;;localhost;d;processed;4
    src.sender;;localhost;d;stamp;1509121934
    src.program;;P-18069;d;processed;1
    src.program;;P-18069;d;stamp;1509121933
    src.program;;P-21491;d;processed;1
    src.program;;P-21491;d;stamp;1509121934
    src.program;;P-9774;d;processed;1
    src.program;;P-9774;d;stamp;1509121919
    src.program;;P-14737;d;processed;1
    src.program;;P-14737;d;stamp;1509121931
    src.host;;localhost;d;processed;4
    src.host;;localhost;d;stamp;1509121934

Questions to reviewers:

What do you think about storing dynamic counters in a separate hash table?
What do you think about the reload-issue? (when the user sets a lower value, after a reload the already registered dynamic counters won't be removed until start-freq timer not expires)?

Iterate through the hashtable items requires to grab the mutex (stats_lock). Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

kira-syslogng · 2017-10-27T20:00:48Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-10-27T20:18:43Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

kira-syslogng · 2017-11-02T12:41:02Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

bodnartibi · 2017-11-03T15:53:27Z

lib/stats/stats-registry.c

+        return NULL;
+      sc = stats_cluster_dynamic_new(sc_key);
+      _insert_cluster(sc);
+      if ( !stats_check_dynamic_clusters_limit(_number_of_dynamic_clusters()))


unnecessary space here

Hmm... we have a tool that checks style and blocks merge until we fix format issues.
So I wouldn't fix it here (fixing astyle would be better as currently we don't check this kind of format issue and it is possible that there are other parts of the code which is affected by the same problem).

bodnartibi · 2017-11-03T16:17:04Z

Looks good to me.

bazsi · 2017-11-04T05:33:13Z

lib/stats/stats-registry.c

+static StatsClusterContainer stats_cluster_container;
+
+static guint
+_number_of_dynamic_clusters()


Missing void in the parameter list

bazsi · 2017-11-04T05:38:16Z

lib/stats/stats-registry.c

+        {
+          msg_warning("Number of dynamic cluster limit has been reached.",
+                      evt_tag_int("allowed clusters", stats_number_of_dynamic_clusters_limit()));
+        }


Please use underscore in the tag name.

Users can set this via the `stats_max_dynamics` global option. This is not the number of counters but clusters which means that users can limit the number of different dynamic value occurrences (as each cluster has a pre-allocated array of counters). Note that after reload, when users set a lower limit, they have to wait for stat-freq-timer being expired to remove the previously allocated dynamic clusters. Dynamic values are * source host (from stats level 2) * sender host (host from) (from stats level 3) * program (from stats level 3) The following example contains 6 different dynamic values (a sender, a host and four different programs): src.sender;;localhost;d;processed;4 src.sender;;localhost;d;stamp;1509121934 src.program;;P-18069;d;processed;1 src.program;;P-18069;d;stamp;1509121933 src.program;;P-21491;d;processed;1 src.program;;P-21491;d;stamp;1509121934 src.program;;P-9774;d;processed;1 src.program;;P-9774;d;stamp;1509121919 src.program;;P-14737;d;processed;1 src.program;;P-14737;d;stamp;1509121931 src.host;;localhost;d;processed;4 src.host;;localhost;d;stamp;1509121934 Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

kira-syslogng · 2017-11-05T21:39:32Z

Build SUCCESS, the tests were executed on test branch: master and test suite: functions

presidento · 2017-11-08T11:51:20Z

lib/stats/stats-cluster.c

@@ -264,6 +264,15 @@ stats_cluster_new(const StatsClusterKey *key)
  return self;
 }

+StatsCluster *
+stats_cluster_dynamic_new(const StatsClusterKey *key)


I rather use a parameter for the constructor.
Based on the signature of this function it seems that we have two different object for dynamic and static clusters, while they differs only in one attribute.

We talked about this IRL, here is my short answer: I think dynamic cluster represents a different (sub)type (it behaves differently than the non-dynamic clusters) and we already provide a different API for dynamic counters (note: there is no dynamic counter, but dynamic cluster, what makes a counter dynamic is that the counter is registered with one of the _dynamic function which track the counter inside a dynamic cluster).
So I think this is not only a simple attribute: this is a kind of type selector and we actually make behavioral decisions based on the value of this attribute.

presidento · 2017-11-08T11:52:13Z

lib/stats/stats-registry.c


-      g_assert(sc->dynamic == dynamic);
-    }
+  if (!sc)


In what circumstances can sc be falsy here?

Okay, I see, in the next commit it can be NULL.

presidento · 2017-11-08T12:07:31Z

lib/cfg-grammar.y

@@ -931,6 +932,7 @@ stat_option
 	: KW_STATS_FREQ '(' nonnegative_integer ')'          { last_stats_options->log_freq = $3; }
 	| KW_STATS_LEVEL '(' nonnegative_integer ')'         { last_stats_options->level = $3; }
 	| KW_STATS_LIFETIME '(' positive_integer ')'      { last_stats_options->lifetime = $3; }
+  | KW_STATS_MAX_DYNAMIC '(' nonnegative_integer ')'   { last_stats_options->max_dynamic = $3; }


What is the default value of this option? What will be on the documentation? As I see:

>0 means the limit of the dynamic stat clusters

0 means disable storing dynamic stat clusters

-1 means there is no limit - but it is not a valid value here...

-1 is cannot be set by users in the config, it is the default value, so when the option is not set then number of dynamic counters are unlimited as before.

lbudai · 2017-11-08T12:50:05Z

@fekete-robert : some documentation related notes

@presidento pointed out that the default value (-1) cannot be set by users, so if someone don't need this feature(->don't want to limit the number of dynamic counters), he or she has to remove the option from the config (and this is intentional from my part)
reload won't clear dynamic counters (hin: lifetime, stats-freq)

fekete-robert · 2017-11-09T18:28:03Z

Thanks, I'll add it to our todo list.

…

On Wed, Nov 8, 2017 at 1:50 PM, Budai Laszlo ***@***.***> wrote: @fekete-robert <https://github.com/fekete-robert> : some documentation related notes - @presidento <https://github.com/presidento> pointed out that the default value (-1) cannot be set by users, so if someone don't need this feature(->don't want to limit the number of dynamic counters), he or she has to remove the option from the config (and this is intentional from my part) - reload won't clear dynamic counters (hin: lifetime, stats-freq) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1743 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADwKbcRH4HsNAm9kQKEMmZU7Dt5Cqj0Gks5s0aN_gaJpZM4QJiG4> .

lbudai added 3 commits October 27, 2017 16:12

stats: do not depend on internal representation of stats clusters

da10182

Iterate through the hashtable items requires to grab the mutex (stats_lock). Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

stats: do not export internal representation

d6fda5c

Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

stats: store dynamic and static clusters in separate hash tables

83522b8

Signed-off-by: Laszlo Budai <laszlo.budai@balabit.com>

lbudai added the in progress label Oct 27, 2017

lbudai force-pushed the limit-dynamic-counters branch from 5fe0a15 to e453bc4 Compare October 27, 2017 20:05

lbudai force-pushed the limit-dynamic-counters branch from e453bc4 to 724cb87 Compare November 2, 2017 12:27

bodnartibi reviewed Nov 3, 2017

View reviewed changes

bazsi reviewed Nov 4, 2017

View reviewed changes

lbudai force-pushed the limit-dynamic-counters branch from 724cb87 to e9d47ab Compare November 5, 2017 21:26

presidento added needs-review and removed in progress labels Nov 8, 2017

presidento self-requested a review November 8, 2017 09:44

presidento reviewed Nov 8, 2017

View reviewed changes

presidento approved these changes Nov 8, 2017

View reviewed changes

presidento added the enhancement label Nov 9, 2017

Kokan approved these changes Nov 15, 2017

View reviewed changes

Kokan merged commit a35f9b4 into syslog-ng:master Nov 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit dynamic counters #1743

Limit dynamic counters #1743

lbudai commented Oct 27, 2017

kira-syslogng commented Oct 27, 2017

kira-syslogng commented Oct 27, 2017

kira-syslogng commented Nov 2, 2017

bodnartibi Nov 3, 2017

lbudai Nov 3, 2017

bodnartibi commented Nov 3, 2017

bazsi Nov 4, 2017

lbudai Nov 5, 2017

bazsi Nov 4, 2017

lbudai Nov 5, 2017

kira-syslogng commented Nov 5, 2017

presidento Nov 8, 2017

lbudai Nov 9, 2017

presidento Nov 8, 2017

presidento Nov 8, 2017

presidento Nov 8, 2017

lbudai Nov 8, 2017

lbudai commented Nov 8, 2017

fekete-robert commented Nov 9, 2017 via email

Limit dynamic counters #1743

Limit dynamic counters #1743

Conversation

lbudai commented Oct 27, 2017

Add config option for limiting dynamic counters

The following example contains 6 different dynamic values (a sender, a host and four different programs):

kira-syslogng commented Oct 27, 2017

kira-syslogng commented Oct 27, 2017

kira-syslogng commented Nov 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bodnartibi commented Nov 3, 2017

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

kira-syslogng commented Nov 5, 2017

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

lbudai commented Nov 8, 2017

fekete-robert commented Nov 9, 2017 via email