Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Limit dynamic counters #1743

Merged
merged 4 commits into from Nov 15, 2017
Merged

Limit dynamic counters #1743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
da10182
stats: do not depend on internal representation of stats clusters
lbudai Oct 27, 2017
d6fda5c
stats: do not export internal representation
lbudai Oct 27, 2017
83522b8
stats: store dynamic and static clusters in separate hash tables
lbudai Oct 27, 2017
e9d47ab
stats: add config option for limiting dynamic counters
lbudai Oct 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/cfg-grammar.y
View file
Open in desktop
Expand Up @@ -192,6 +192,7 @@ extern struct _StatsOptions *last_stats_options;
%token KW_MARK_MODE 10081
%token KW_ENCODING 10082
%token KW_TYPE 10083
%token KW_STATS_MAX_DYNAMIC 10084

%token KW_CHAIN_HOSTNAMES 10090
%token KW_NORMALIZE_HOSTNAMES 10091
Expand Down Expand Up @@ -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; }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-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.

;

dns_cache_option
Expand Down
1 change: 1 addition & 0 deletions lib/cfg-parser.c
View file
Open in desktop
Expand Up @@ -86,6 +86,7 @@ static CfgLexerKeyword main_keywords[] =
{ "stats_lifetime", KW_STATS_LIFETIME },
{ "stats_level", KW_STATS_LEVEL },
{ "stats", KW_STATS_FREQ, KWS_OBSOLETE, "stats_freq" },
{ "stats_max_dynamics", KW_STATS_MAX_DYNAMIC },
{ "flush_lines", KW_FLUSH_LINES },
{ "flush_timeout", KW_FLUSH_TIMEOUT },
{ "suppress", KW_SUPPRESS },
Expand Down
9 changes: 9 additions & 0 deletions lib/stats/stats-cluster.c
View file
Open in desktop
Expand Up @@ -264,6 +264,15 @@ stats_cluster_new(const StatsClusterKey *key)
return self;
}

StatsCluster *
stats_cluster_dynamic_new(const StatsClusterKey *key)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
StatsCluster *sc = stats_cluster_new(key);
sc->dynamic = TRUE;

return sc;
}

void
stats_counter_group_free(StatsCounterGroup *self)
{
Expand Down
1 change: 1 addition & 0 deletions lib/stats/stats-cluster.h
View file
Open in desktop
Expand Up @@ -147,6 +147,7 @@ gboolean stats_cluster_is_alive(StatsCluster *self, gint type);
gboolean stats_cluster_is_indexed(StatsCluster *self, gint type);

StatsCluster *stats_cluster_new(const StatsClusterKey *key);
StatsCluster *stats_cluster_dynamic_new(const StatsClusterKey *key);
void stats_cluster_free(StatsCluster *self);

void stats_cluster_key_set(StatsClusterKey *self, guint16 component, const gchar *id, const gchar *instance, StatsCounterGroupInit counter_group_ctor);
Expand Down
15 changes: 4 additions & 11 deletions lib/stats/stats-query.c
View file
Open in desktop
Expand Up @@ -125,25 +125,18 @@ _index_counter(StatsCluster *sc, gint type, StatsCounterItem *counter, gpointer
}

static void
_update_indexes_of_cluster_if_needed(gpointer key, gpointer value)
_update_indexes_of_cluster_if_needed(StatsCluster *sc, gpointer user_data)
{
StatsCluster *sc = (StatsCluster *)key;
stats_cluster_foreach_counter(sc, _index_counter, NULL);
}

static void
_update_index(void)
{
GHashTable *counter_container = stats_registry_get_container();
gpointer key, value;
GHashTableIter iter;

g_static_mutex_lock(&stats_query_mutex);
g_hash_table_iter_init(&iter, counter_container);
while (g_hash_table_iter_next(&iter, &key, &value))
{
_update_indexes_of_cluster_if_needed(key, value);
}
stats_lock();
stats_foreach_cluster(_update_indexes_of_cluster_if_needed, NULL);
stats_unlock();
g_static_mutex_unlock(&stats_query_mutex);
}

Expand Down
110 changes: 83 additions & 27 deletions lib/stats/stats-registry.c
View file
Open in desktop
Expand Up @@ -26,14 +26,30 @@
#include "cfg.h"
#include <string.h>

static GHashTable *stats_cluster_container;
typedef struct _StatsClusterContainer
{
GHashTable *static_clusters;
GHashTable *dynamic_clusters;
} StatsClusterContainer;

static StatsClusterContainer stats_cluster_container;

static guint
_number_of_dynamic_clusters(void)
{
return g_hash_table_size(stats_cluster_container.dynamic_clusters);
}

static GStaticMutex stats_mutex = G_STATIC_MUTEX_INIT;
gboolean stats_locked;

static void
_insert_cluster(StatsCluster *sc)
{
g_hash_table_insert(stats_cluster_container, &sc->key, sc);
if (sc->dynamic)
g_hash_table_insert(stats_cluster_container.dynamic_clusters, &sc->key, sc);
else
g_hash_table_insert(stats_cluster_container.static_clusters, &sc->key, sc);
}

void
Expand All @@ -51,31 +67,65 @@ stats_unlock(void)
}

static StatsCluster *
_grab_cluster(gint stats_level, const StatsClusterKey *sc_key, gboolean dynamic)
_grab_dynamic_cluster(const StatsClusterKey *sc_key)
{
StatsCluster *sc;

if (!stats_check_level(stats_level))
return NULL;
sc = g_hash_table_lookup(stats_cluster_container.dynamic_clusters, sc_key);
if (!sc)
{
if (!stats_check_dynamic_clusters_limit(_number_of_dynamic_clusters()))
return NULL;
sc = stats_cluster_dynamic_new(sc_key);
_insert_cluster(sc);
if ( !stats_check_dynamic_clusters_limit(_number_of_dynamic_clusters()))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary space here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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

return sc;

}

static StatsCluster *
_grab_static_cluster(const StatsClusterKey *sc_key)
{
StatsCluster *sc;

sc = g_hash_table_lookup(stats_cluster_container, sc_key);
sc = g_hash_table_lookup(stats_cluster_container.static_clusters, sc_key);
if (!sc)
{
/* no such StatsCluster instance, register one */
sc = stats_cluster_new(sc_key);
sc->dynamic = dynamic;
_insert_cluster(sc);
}

return sc;
}

static StatsCluster *
_grab_cluster(gint stats_level, const StatsClusterKey *sc_key, gboolean dynamic)
{
if (!stats_check_level(stats_level))
return NULL;

StatsCluster *sc = NULL;

if (dynamic)
sc = _grab_dynamic_cluster(sc_key);
else
{
/* check that we are not overwriting a dynamic counter with a
* non-dynamic one or vica versa. This could only happen if the same
* key is used for both a dynamic counter and a non-dynamic one, which
* is a programming error */
sc = _grab_static_cluster(sc_key);

g_assert(sc->dynamic == dynamic);
}
if (!sc)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In what circumstances can sc be falsy here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return NULL;

/* check that we are not overwriting a dynamic counter with a
* non-dynamic one or vica versa. This could only happen if the same
* key is used for both a dynamic counter and a non-dynamic one, which
* is a programming error */

g_assert(sc->dynamic == dynamic);
return sc;
}

Expand Down Expand Up @@ -156,6 +206,8 @@ stats_register_and_increment_dynamic_counter(gint stats_level, const StatsCluste

g_assert(stats_locked);
handle = stats_register_dynamic_counter(stats_level, sc_key, SC_TYPE_PROCESSED, &counter);
if (!handle)
return;
stats_counter_inc(counter);
if (timestamp >= 0)
{
Expand Down Expand Up @@ -199,7 +251,7 @@ stats_unregister_counter(const StatsClusterKey *sc_key, gint type,
if (*counter == NULL)
return;

sc = g_hash_table_lookup(stats_cluster_container, sc_key);
sc = g_hash_table_lookup(stats_cluster_container.static_clusters, sc_key);

stats_cluster_untrack_counter(sc, type, counter);
}
Expand Down Expand Up @@ -251,7 +303,8 @@ stats_foreach_cluster(StatsForeachClusterFunc func, gpointer user_data)
gpointer args[] = { func, user_data };

g_assert(stats_locked);
g_hash_table_foreach(stats_cluster_container, _foreach_cluster_helper, args);
g_hash_table_foreach(stats_cluster_container.static_clusters, _foreach_cluster_helper, args);
g_hash_table_foreach(stats_cluster_container.dynamic_clusters, _foreach_cluster_helper, args);
}

static gboolean
Expand All @@ -269,7 +322,8 @@ void
stats_foreach_cluster_remove(StatsForeachClusterRemoveFunc func, gpointer user_data)
{
gpointer args[] = { func, user_data };
g_hash_table_foreach_remove(stats_cluster_container, _foreach_cluster_remove_helper, args);
g_hash_table_foreach_remove(stats_cluster_container.static_clusters, _foreach_cluster_remove_helper, args);
g_hash_table_foreach_remove(stats_cluster_container.dynamic_clusters, _foreach_cluster_remove_helper, args);
}

static void
Expand All @@ -294,21 +348,23 @@ stats_foreach_counter(StatsForeachCounterFunc func, gpointer user_data)
void
stats_registry_init(void)
{
stats_cluster_container = g_hash_table_new_full((GHashFunc) stats_cluster_hash, (GEqualFunc) stats_cluster_equal, NULL,
(GDestroyNotify) stats_cluster_free);
stats_cluster_container.static_clusters = g_hash_table_new_full((GHashFunc) stats_cluster_hash,
(GEqualFunc) stats_cluster_equal, NULL,
(GDestroyNotify) stats_cluster_free);
stats_cluster_container.dynamic_clusters = g_hash_table_new_full((GHashFunc) stats_cluster_hash,
(GEqualFunc) stats_cluster_equal, NULL,
(GDestroyNotify) stats_cluster_free);

g_static_mutex_init(&stats_mutex);
}

void
stats_registry_deinit(void)
{
g_hash_table_destroy(stats_cluster_container);
stats_cluster_container = NULL;
g_hash_table_destroy(stats_cluster_container.static_clusters);
g_hash_table_destroy(stats_cluster_container.dynamic_clusters);
stats_cluster_container.static_clusters = NULL;
stats_cluster_container.dynamic_clusters = NULL;
g_static_mutex_free(&stats_mutex);
}

GHashTable *
stats_registry_get_container(void)
{
return stats_cluster_container;
}
5 changes: 3 additions & 2 deletions lib/stats/stats-registry.h
View file
Open in desktop
Expand Up @@ -50,9 +50,10 @@ void stats_foreach_cluster_remove(StatsForeachClusterRemoveFunc func, gpointer u
void stats_registry_init(void);
void stats_registry_deinit(void);

GHashTable* stats_registry_get_container(void);

void save_counter_to_persistent_storage(GlobalConfig *cfg, StatsCounterItem *counter);
void load_counter_from_persistent_storage(GlobalConfig *cfg, StatsCounterItem *counter);

gboolean stats_check_dynamic_clusters_limit(guint number_of_clusters);
gint stats_number_of_dynamic_clusters_limit(void);

#endif
20 changes: 20 additions & 0 deletions lib/stats/stats.c
View file
Open in desktop
Expand Up @@ -257,6 +257,7 @@ stats_options_defaults(StatsOptions *options)
options->level = 0;
options->log_freq = 600;
options->lifetime = 600;
options->max_dynamic = -1;
}

gboolean
Expand All @@ -267,3 +268,22 @@ stats_check_level(gint level)
else
return level == 0;
}

gboolean
stats_check_dynamic_clusters_limit(guint number_of_clusters)
{
if (!stats_options)
return TRUE;
if (stats_options->max_dynamic == -1)
return TRUE;
return (stats_options->max_dynamic > number_of_clusters);
}

gint
stats_number_of_dynamic_clusters_limit()
{
if (!stats_options)
return -1;
return stats_options->max_dynamic;
}

2 changes: 1 addition & 1 deletion lib/stats/stats.h
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ typedef struct _StatsOptions
gint log_freq;
gint level;
gint lifetime;
gint max_dynamic;
} StatsOptions;

enum
Expand All @@ -49,6 +50,5 @@ void stats_destroy(void);

void stats_options_defaults(StatsOptions *options);


#endif

8 changes: 7 additions & 1 deletion lib/stats/tests/Makefile.am
View file
Open in desktop
Expand Up @@ -14,9 +14,15 @@ stats_test_extra_modules = \
$(PREOPEN_SYSLOGFORMAT)

lib_stats_tests_TESTS += \
lib/stats/tests/test_stats_query
lib/stats/tests/test_stats_query \
lib/stats/tests/test_dynamic_ctr_reg

lib_stats_tests_test_stats_query_CFLAGS = $(TEST_CFLAGS)
lib_stats_tests_test_stats_query_LDADD = \
$(TEST_LDADD) $(stats_test_extra_modules)

lib_stats_tests_test_dynamic_ctr_reg_CFLAGS = $(TEST_CFLAGS)
lib_stats_tests_test_dynamic_ctr_reg_LDADD = \
$(TEST_LDADD) $(stats_test_extra_modules)

endif