From e4a294d367989dcda2273e9cb209f4959efd88cb Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Thu, 14 Dec 2023 12:54:42 +0100 Subject: [PATCH 1/8] daemon: Add `metric_family_compare` and `label_set_compare`. --- src/daemon/metric.c | 39 ++++++++++++++++++++++++++++++++++++--- src/daemon/metric.h | 19 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/daemon/metric.c b/src/daemon/metric.c index 9bc0c67452..276f3e50b5 100644 --- a/src/daemon/metric.c +++ b/src/daemon/metric.c @@ -59,7 +59,7 @@ int value_marshal_text(strbuf_t *buf, value_t v, metric_type_t type) { } } -static int label_pair_compare(void const *a, void const *b) { +static int label_name_compare(void const *a, void const *b) { return strcmp(((label_pair_t const *)a)->name, ((label_pair_t const *)b)->name); } @@ -79,7 +79,7 @@ static label_pair_t *label_set_read(label_set_t const *labels, }; label_pair_t *ret = bsearch(&label, labels->ptr, labels->num, - sizeof(*labels->ptr), label_pair_compare); + sizeof(*labels->ptr), label_name_compare); if (ret == NULL) { errno = ENOENT; return NULL; @@ -132,7 +132,7 @@ int label_set_add(label_set_t *labels, char const *name, char const *value) { labels->ptr[labels->num] = pair; labels->num++; - qsort(labels->ptr, labels->num, sizeof(*labels->ptr), label_pair_compare); + qsort(labels->ptr, labels->num, sizeof(*labels->ptr), label_name_compare); return 0; } @@ -688,3 +688,36 @@ metric_t *metric_parse_identity(char const *buf) { return fam->metric.ptr; } + +static int label_pair_compare(label_pair_t a, label_pair_t b) { + int cmp = strcmp(a.name, b.name); + if (cmp != 0) { + return cmp; + } + + return strcmp(a.value, b.value); +} + +int label_set_compare(label_set_t a, label_set_t b) { + if (a.num != b.num) { + return a.num < b.num ? -1 : 1; + } + + for (size_t i = 0; i < a.num; i++) { + int cmp = label_pair_compare(a.ptr[i], b.ptr[i]); + if (cmp != 0) { + return cmp; + } + } + + return 0; +} + +int metric_family_compare(metric_family_t const *a, metric_family_t const *b) { + int cmp = strcmp(a->name, b->name); + if (cmp != 0) { + return cmp; + } + + return label_set_compare(a->resource, b->resource); +} diff --git a/src/daemon/metric.h b/src/daemon/metric.h index 16289d3457..dc422a94c7 100644 --- a/src/daemon/metric.h +++ b/src/daemon/metric.h @@ -83,6 +83,15 @@ int label_set_add(label_set_t *labels, char const *name, char const *value); * initializes the label set to zero. */ void label_set_reset(label_set_t *labels); +/* label_set_compare compares two label sets. It returns an integer indicating + * the result of the comparison, as follows: + * + * - 0, if the a and b are equal; + * - a negative value if a is less than b; + * - a positive value if a is greater than b. + */ +int label_set_compare(label_set_t a, label_set_t b); + /* * Metric */ @@ -190,4 +199,14 @@ void metric_family_free(metric_family_t *fam); * metric_family_free(). */ metric_family_t *metric_family_clone(metric_family_t const *fam); +/* metric_family_compare compares two metric families, taking into account the + * metric family name and any resource attributes. It returns an integer + * indicating the result of the comparison, as follows: + * + * - 0, if the a and b are equal; + * - a negative value if a is less than b; + * - a positive value if a is greater than b. + */ +int metric_family_compare(metric_family_t const *a, metric_family_t const *b); + #endif From 286974d80cce1d4b759afa6b71bef0b1b7f31e4a Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 16 Dec 2023 08:26:26 +0100 Subject: [PATCH 2/8] New utility: resource_metrics. Resource metrics allows plugin to stage metric families, grouped by resource. This is designed to work well with plugins that export the OpenTelemetry protocol. --- src/utils/resource_metrics/resource_metrics.c | 202 ++++++++++++++++++ src/utils/resource_metrics/resource_metrics.h | 54 +++++ 2 files changed, 256 insertions(+) create mode 100644 src/utils/resource_metrics/resource_metrics.c create mode 100644 src/utils/resource_metrics/resource_metrics.h diff --git a/src/utils/resource_metrics/resource_metrics.c b/src/utils/resource_metrics/resource_metrics.c new file mode 100644 index 0000000000..e808030579 --- /dev/null +++ b/src/utils/resource_metrics/resource_metrics.c @@ -0,0 +1,202 @@ +/** + * collectd - src/utils/resource_metrics/resource_metrics.c + * Copyright (C) 2023 Florian octo Forster + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Authors: + * Florian octo Forster + **/ + +#include "utils/resource_metrics/resource_metrics.h" +#include "daemon/plugin.h" +#include "utils/common/common.h" + +static int resource_metrics_compare(resource_metrics_t const *a, + resource_metrics_t const *b) { + return label_set_compare(a->resource, b->resource); +} + +static resource_metrics_t *lookup_resource(resource_metrics_set_t *set, + label_set_t resource) { + resource_metrics_t key = { + .resource = resource, + }; + + return bsearch(&key, set->ptr, set->num, sizeof(*set->ptr), + (void *)resource_metrics_compare); +} + +static int insert_resource(resource_metrics_set_t *set, label_set_t resource) { + resource_metrics_t *rm = + realloc(set->ptr, (set->num + 1) * sizeof(*set->ptr)); + if (rm == NULL) { + return ENOMEM; + } + set->ptr = rm; + + rm = set->ptr + set->num; + memset(rm, 0, sizeof(*rm)); + + int status = label_set_clone(&rm->resource, resource); + if (status != 0) { + return ENOMEM; + } + set->num++; + + qsort(set->ptr, set->num, sizeof(*set->ptr), + (void *)resource_metrics_compare); + return 0; +} + +static resource_metrics_t * +lookup_or_insert_resource(resource_metrics_set_t *set, label_set_t resource) { + resource_metrics_t *ret = lookup_resource(set, resource); + if (ret != NULL) { + return ret; + } + + int status = insert_resource(set, resource); + if (status != 0) { + ERROR("resource_metrics: insert_resource failed: %s", STRERROR(status)); + return NULL; + } + + return lookup_resource(set, resource); +} + +static int compare_family_by_name(metric_family_t **a, metric_family_t **b) { + return strcmp((*a)->name, (*b)->name); +} + +static metric_family_t *lookup_family(resource_metrics_t *rm, + metric_family_t const *fam) { + return bsearch(&fam, rm->families, rm->families_num, sizeof(*rm->families), + (void *)compare_family_by_name); +} + +static int insert_family(resource_metrics_t *rm, metric_family_t const *fam) { + metric_family_t **tmp = + realloc(rm->families, (rm->families_num + 1) * sizeof(*rm->families)); + if (tmp == NULL) { + return ENOMEM; + } + rm->families = tmp; + + /* NOTE: metric_family_clone also copies the resource attributes. This is not + * strictly required, since we have these attributes in rm. We keep the copies + * for now for the sake of simplicity. If memory consumption is a problem, + * this could be de-duplicated, at the cost of more complicated memory + * management. */ + rm->families[rm->families_num] = metric_family_clone(fam); + if (rm->families[rm->families_num] == NULL) { + return errno; + } + rm->families_num++; + + return 0; +} + +static metric_family_t *lookup_or_insert_family(resource_metrics_t *rm, + metric_family_t const *fam) { + metric_family_t *ret = lookup_family(rm, fam); + if (ret != NULL) { + return ret; + } + + int status = insert_family(rm, fam); + if (status != 0) { + ERROR("resource_metrics: insert_family failed: %s", STRERROR(status)); + return NULL; + } + + return lookup_family(rm, fam); +} + +static int compare_metrics(metric_t const *a, metric_t const *b) { + return label_set_compare(a->label, b->label); +} + +static bool metric_exists(metric_family_t const *fam, metric_t const *m) { + metric_family_t *found = + bsearch(m, fam->metric.ptr, fam->metric.num, sizeof(*fam->metric.ptr), + (void *)compare_metrics); + return found != NULL; +} + +static int insert_metrics(metric_family_t *fam, metric_list_t metrics) { + for (size_t i = 0; i < metrics.num; i++) { + if (metric_exists(fam, metrics.ptr + i)) { + return EEXIST; + } + } + + int last_err = 0; + for (size_t i = 0; i < metrics.num; i++) { + int status = metric_family_metric_append(fam, metrics.ptr[i]); + if (status != 0) { + ERROR("resource_metrics: metric_family_metric_append failed: %s", STRERROR(status)); + /* DO NOT RETURN: the metric list may be unsorted */ + last_err = status; + } + } + + qsort(fam->metric.ptr, fam->metric.num, sizeof(*fam->metric.ptr), + (void *)compare_metrics); + return last_err; +} + +int resource_metrics_add(resource_metrics_set_t *set, + metric_family_t const *fam) { + if (set == NULL || fam == NULL) { + return EINVAL; + } + + resource_metrics_t *rm = lookup_or_insert_resource(set, fam->resource); + if (rm == NULL) { + return -1; + } + + metric_family_t *staged_fam = lookup_or_insert_family(rm, fam); + if (staged_fam == NULL) { + return -1; + } + + return insert_metrics(staged_fam, fam->metric); +} + +static void resource_reset(resource_metrics_t *rm) { + label_set_reset(&rm->resource); + + for (size_t i = 0; i < rm->families_num; i++) { + metric_family_free(rm->families[i]); + rm->families[i] = NULL; + } + free(rm->families); + rm->families = NULL; + rm->families_num = 0; +} + +void resource_metrics_reset(resource_metrics_set_t *set) { + for (size_t i = 0; i < set->num; i++) { + resource_reset(&set->ptr[i]); + } + set->ptr = NULL; + set->num = 0; +} diff --git a/src/utils/resource_metrics/resource_metrics.h b/src/utils/resource_metrics/resource_metrics.h new file mode 100644 index 0000000000..26293bdbc5 --- /dev/null +++ b/src/utils/resource_metrics/resource_metrics.h @@ -0,0 +1,54 @@ +/** + * collectd - src/utils/resource_metrics/resource_metrics.h + * Copyright (C) 2023 Florian octo Forster + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Authors: + * Florian octo Forster + **/ + +#ifndef UTILS_RESOURCE_METRICS_H +#define UTILS_RESOURCE_METRICS_H 1 + +#include "collectd.h" +#include "daemon/metric.h" + +typedef struct { + label_set_t resource; + + metric_family_t **families; + size_t families_num; +} resource_metrics_t; + +typedef struct { + resource_metrics_t *ptr; + size_t num; +} resource_metrics_set_t; + +/* resource_metrics_add adds a metric family to the resource metrics set. + * If any metric within the metric family is already part of the resource + * metrics set, the function will return EEXIST and rm remains unmodified. */ +int resource_metrics_add(resource_metrics_set_t *rm, metric_family_t const *fam); + +/* resource_metrics_reset frees all the memory held inside the set. set itself + * is not freed and can be reused afterwards. */ +void resource_metrics_reset(resource_metrics_set_t *set); + +#endif From 2e989a90203d5f589935efbb77d1c8d0cf994762 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 16 Dec 2023 12:53:53 +0100 Subject: [PATCH 3/8] resource_metrics: Add unit test. --- Makefile.am | 8 ++ .../resource_metrics/resource_metrics_test.c | 135 ++++++++++++++++++ 2 files changed, 143 insertions(+) create mode 100644 src/utils/resource_metrics/resource_metrics_test.c diff --git a/Makefile.am b/Makefile.am index 9e610e337b..29779a82be 100644 --- a/Makefile.am +++ b/Makefile.am @@ -159,6 +159,7 @@ check_PROGRAMS = \ test_utils_latency \ test_utils_message_parser \ test_utils_mount \ + test_utils_resource_metrics \ test_utils_strbuf \ test_utils_subst \ test_utils_time \ @@ -383,6 +384,13 @@ test_utils_message_parser_SOURCES = \ test_utils_message_parser_CPPFLAGS = $(AM_CPPFLAGS) test_utils_message_parser_LDADD = liboconfig.la libplugin_mock.la -lm +test_utils_resource_metrics_SOURCES = \ + src/utils/resource_metrics/resource_metrics_test.c \ + src/utils/resource_metrics/resource_metrics.c \ + src/utils/resource_metrics/resource_metrics.h \ + src/testing.h +test_utils_resource_metrics_LDADD = libmetric.la libplugin_mock.la + test_utils_time_SOURCES = \ src/daemon/utils_time_test.c \ src/testing.h diff --git a/src/utils/resource_metrics/resource_metrics_test.c b/src/utils/resource_metrics/resource_metrics_test.c new file mode 100644 index 0000000000..bbf5bc2d3a --- /dev/null +++ b/src/utils/resource_metrics/resource_metrics_test.c @@ -0,0 +1,135 @@ +/** + * collectd - resource_metrics_test.c + * Copyright (C) 2023 Florian octo Forster + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Authors: + * Florian octo Forster + */ + +#include "testing.h" +#include "utils/resource_metrics/resource_metrics.h" + +static metric_family_t *make_metric_family(int resource, int family, + int metric) { + metric_family_t *fam = calloc(1, sizeof(*fam)); + + char name[64] = {0}; + snprintf(name, sizeof(name), "family%d", family); + fam->name = strdup(name); + + snprintf(name, sizeof(name), "resource%d", resource); + metric_family_resource_attribute_update(fam, "service.name", name); + + snprintf(name, sizeof(name), "metric%d", metric); + metric_family_append(fam, "metric.name", name, (value_t){.gauge = 42}, NULL); + + return fam; +} + +static size_t count_metrics(resource_metrics_set_t set) { + size_t sum = 0; + for (size_t i = 0; i < set.num; i++) { + resource_metrics_t const *rm = set.ptr + i; + for (size_t j = 0; j < rm->families_num; j++) { + sum += rm->families[j]->metric.num; + } + } + return sum; +} + +DEF_TEST(resource_metrics_add) { + resource_metrics_set_t set = {0}; + metric_family_t *fam; + + CHECK_NOT_NULL(fam = make_metric_family(1, 1, 1)); + CHECK_ZERO(resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(1, set.num); + EXPECT_EQ_INT(1, count_metrics(set)); + /* adding the same familiy twice should return EEXIST. */ + EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(1, set.num); + EXPECT_EQ_INT(1, count_metrics(set)); + metric_family_free(fam); + + /* adding the same metric (but with a different resource attribute) should + * succeed. */ + CHECK_NOT_NULL(fam = make_metric_family(2, 1, 1)); + CHECK_ZERO(resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(2, count_metrics(set)); + /* adding the same familiy twice should return EEXIST. */ + EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(2, count_metrics(set)); + metric_family_free(fam); + + /* adding a different metric family to an existing resource should work. */ + CHECK_NOT_NULL(fam = make_metric_family(1, 2, 1)); + CHECK_ZERO(resource_metrics_add(&set, fam)); + /* reuses existing resource */ + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(3, count_metrics(set)); + /* adding the same familiy twice should return EEXIST. */ + EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(3, count_metrics(set)); + metric_family_free(fam); + + /* adding a different metric to an existing metric family should work. */ + CHECK_NOT_NULL(fam = make_metric_family(1, 1, 2)); + CHECK_ZERO(resource_metrics_add(&set, fam)); + /* reuses existing resource */ + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(4, count_metrics(set)); + /* adding the same familiy twice should return EEXIST. */ + EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(4, count_metrics(set)); + metric_family_free(fam); + + resource_metrics_reset(&set); + + /* adding 1000 distinct metrics. */ + size_t want_metrics_count = 0; + for (int i = 0; i < 10; i++) { + for (int j = 0; j < 10; j++) { + for (int k = 0; k < 10; k++) { + /* add metrics in "random" order */ + CHECK_NOT_NULL( + fam = make_metric_family((i * 7) % 10, (j * 7) % 10, (k * 7) % 10)); + EXPECT_EQ_INT(0, resource_metrics_add(&set, fam)); + want_metrics_count++; + EXPECT_EQ_INT(want_metrics_count, count_metrics(set)); + metric_family_free(fam); + } + } + EXPECT_EQ_INT(i + 1, set.num); + } + + resource_metrics_reset(&set); + return 0; +} + +int main(void) { + RUN_TEST(resource_metrics_add); + + END_TEST; +} From 584c67fbecdfad71e456f73bc361975b1240273a Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 16 Dec 2023 12:58:37 +0100 Subject: [PATCH 4/8] resource_metrics: Fix bugs surfaced by the unit test. * Sort metric families after inserting. * Dereference the pointer-pointer returned by `bsearch` in `lookup_family`. * Delete the metrics created by `metric_family_clone`. * Run ./contrib/format.sh --- src/utils/resource_metrics/resource_metrics.c | 28 +++++++++++++++---- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/src/utils/resource_metrics/resource_metrics.c b/src/utils/resource_metrics/resource_metrics.c index e808030579..dafa08a3a1 100644 --- a/src/utils/resource_metrics/resource_metrics.c +++ b/src/utils/resource_metrics/resource_metrics.c @@ -24,9 +24,9 @@ * Florian octo Forster **/ -#include "utils/resource_metrics/resource_metrics.h" #include "daemon/plugin.h" #include "utils/common/common.h" +#include "utils/resource_metrics/resource_metrics.h" static int resource_metrics_compare(resource_metrics_t const *a, resource_metrics_t const *b) { @@ -78,7 +78,9 @@ lookup_or_insert_resource(resource_metrics_set_t *set, label_set_t resource) { return NULL; } - return lookup_resource(set, resource); + ret = lookup_resource(set, resource); + assert(ret != NULL); + return ret; } static int compare_family_by_name(metric_family_t **a, metric_family_t **b) { @@ -87,8 +89,13 @@ static int compare_family_by_name(metric_family_t **a, metric_family_t **b) { static metric_family_t *lookup_family(resource_metrics_t *rm, metric_family_t const *fam) { - return bsearch(&fam, rm->families, rm->families_num, sizeof(*rm->families), - (void *)compare_family_by_name); + metric_family_t **ret = + bsearch(&fam, rm->families, rm->families_num, sizeof(*rm->families), + (void *)compare_family_by_name); + if (ret == NULL) { + return NULL; + } + return *ret; } static int insert_family(resource_metrics_t *rm, metric_family_t const *fam) { @@ -108,8 +115,14 @@ static int insert_family(resource_metrics_t *rm, metric_family_t const *fam) { if (rm->families[rm->families_num] == NULL) { return errno; } + + metric_family_metric_reset(rm->families[rm->families_num]); + label_set_reset(&rm->families[rm->families_num]->resource); + rm->families_num++; + qsort(rm->families, rm->families_num, sizeof(*rm->families), + (void *)compare_family_by_name); return 0; } @@ -126,7 +139,9 @@ static metric_family_t *lookup_or_insert_family(resource_metrics_t *rm, return NULL; } - return lookup_family(rm, fam); + ret = lookup_family(rm, fam); + assert(ret != NULL); + return ret; } static int compare_metrics(metric_t const *a, metric_t const *b) { @@ -151,7 +166,8 @@ static int insert_metrics(metric_family_t *fam, metric_list_t metrics) { for (size_t i = 0; i < metrics.num; i++) { int status = metric_family_metric_append(fam, metrics.ptr[i]); if (status != 0) { - ERROR("resource_metrics: metric_family_metric_append failed: %s", STRERROR(status)); + ERROR("resource_metrics: metric_family_metric_append failed: %s", + STRERROR(status)); /* DO NOT RETURN: the metric list may be unsorted */ last_err = status; } From c58b9e0187bac9eacde9e29bb9987774ee48241d Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 16 Dec 2023 12:59:03 +0100 Subject: [PATCH 5/8] resource_metrics: Doc comment improvements. --- src/utils/resource_metrics/resource_metrics.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/utils/resource_metrics/resource_metrics.h b/src/utils/resource_metrics/resource_metrics.h index 26293bdbc5..ffda8e14c5 100644 --- a/src/utils/resource_metrics/resource_metrics.h +++ b/src/utils/resource_metrics/resource_metrics.h @@ -37,15 +37,20 @@ typedef struct { size_t families_num; } resource_metrics_t; +/* resource_metrics_set_t is a set of metric families, grouped by resource + * attributes. Because the resource attributes are kept track of in + * resource_metrics_t, the metric_family_t.resource field is cleared and cannot + * be used. */ typedef struct { resource_metrics_t *ptr; size_t num; } resource_metrics_set_t; -/* resource_metrics_add adds a metric family to the resource metrics set. +/* resource_metrics_add copies a metric family to the resource metrics set. * If any metric within the metric family is already part of the resource * metrics set, the function will return EEXIST and rm remains unmodified. */ -int resource_metrics_add(resource_metrics_set_t *rm, metric_family_t const *fam); +int resource_metrics_add(resource_metrics_set_t *rm, + metric_family_t const *fam); /* resource_metrics_reset frees all the memory held inside the set. set itself * is not freed and can be reused afterwards. */ From 69f77119a54cff4ab4469dd9cafe1cb0cbdb1e51 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sat, 16 Dec 2023 14:29:00 +0100 Subject: [PATCH 6/8] resource metrics: Add missing free to `resource_metrics_reset`. --- src/utils/resource_metrics/resource_metrics.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/utils/resource_metrics/resource_metrics.c b/src/utils/resource_metrics/resource_metrics.c index dafa08a3a1..4f63167717 100644 --- a/src/utils/resource_metrics/resource_metrics.c +++ b/src/utils/resource_metrics/resource_metrics.c @@ -213,6 +213,7 @@ void resource_metrics_reset(resource_metrics_set_t *set) { for (size_t i = 0; i < set->num; i++) { resource_reset(&set->ptr[i]); } + free(set->ptr); set->ptr = NULL; set->num = 0; } From a529e36b09a7a1e24a723d0a36b31fdf1e5eef78 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sun, 17 Dec 2023 09:19:00 +0100 Subject: [PATCH 7/8] resource_metrics: Treat metrics with different time stamps as different metrics. --- src/utils/resource_metrics/resource_metrics.c | 13 +++++++- .../resource_metrics/resource_metrics_test.c | 30 ++++++++++++++----- 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/src/utils/resource_metrics/resource_metrics.c b/src/utils/resource_metrics/resource_metrics.c index 4f63167717..b5eca7f831 100644 --- a/src/utils/resource_metrics/resource_metrics.c +++ b/src/utils/resource_metrics/resource_metrics.c @@ -145,7 +145,18 @@ static metric_family_t *lookup_or_insert_family(resource_metrics_t *rm, } static int compare_metrics(metric_t const *a, metric_t const *b) { - return label_set_compare(a->label, b->label); + int cmp = label_set_compare(a->label, b->label); + if (cmp != 0) { + return cmp; + } + + if (a->time < b->time) { + return -1; + } else if (a->time > b->time) { + return 1; + } + + return 0; } static bool metric_exists(metric_family_t const *fam, metric_t const *m) { diff --git a/src/utils/resource_metrics/resource_metrics_test.c b/src/utils/resource_metrics/resource_metrics_test.c index bbf5bc2d3a..859b660f7c 100644 --- a/src/utils/resource_metrics/resource_metrics_test.c +++ b/src/utils/resource_metrics/resource_metrics_test.c @@ -27,8 +27,8 @@ #include "testing.h" #include "utils/resource_metrics/resource_metrics.h" -static metric_family_t *make_metric_family(int resource, int family, - int metric) { +static metric_family_t *make_metric_family(int resource, int family, int metric, + int time) { metric_family_t *fam = calloc(1, sizeof(*fam)); char name[64] = {0}; @@ -41,6 +41,8 @@ static metric_family_t *make_metric_family(int resource, int family, snprintf(name, sizeof(name), "metric%d", metric); metric_family_append(fam, "metric.name", name, (value_t){.gauge = 42}, NULL); + fam->metric.ptr[0].time = (cdtime_t)time; + return fam; } @@ -59,7 +61,7 @@ DEF_TEST(resource_metrics_add) { resource_metrics_set_t set = {0}; metric_family_t *fam; - CHECK_NOT_NULL(fam = make_metric_family(1, 1, 1)); + CHECK_NOT_NULL(fam = make_metric_family(1, 1, 1, 1)); CHECK_ZERO(resource_metrics_add(&set, fam)); EXPECT_EQ_INT(1, set.num); EXPECT_EQ_INT(1, count_metrics(set)); @@ -71,7 +73,7 @@ DEF_TEST(resource_metrics_add) { /* adding the same metric (but with a different resource attribute) should * succeed. */ - CHECK_NOT_NULL(fam = make_metric_family(2, 1, 1)); + CHECK_NOT_NULL(fam = make_metric_family(2, 1, 1, 1)); CHECK_ZERO(resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(2, count_metrics(set)); @@ -82,7 +84,7 @@ DEF_TEST(resource_metrics_add) { metric_family_free(fam); /* adding a different metric family to an existing resource should work. */ - CHECK_NOT_NULL(fam = make_metric_family(1, 2, 1)); + CHECK_NOT_NULL(fam = make_metric_family(1, 2, 1, 1)); CHECK_ZERO(resource_metrics_add(&set, fam)); /* reuses existing resource */ EXPECT_EQ_INT(2, set.num); @@ -94,7 +96,7 @@ DEF_TEST(resource_metrics_add) { metric_family_free(fam); /* adding a different metric to an existing metric family should work. */ - CHECK_NOT_NULL(fam = make_metric_family(1, 1, 2)); + CHECK_NOT_NULL(fam = make_metric_family(1, 1, 2, 1)); CHECK_ZERO(resource_metrics_add(&set, fam)); /* reuses existing resource */ EXPECT_EQ_INT(2, set.num); @@ -105,6 +107,18 @@ DEF_TEST(resource_metrics_add) { EXPECT_EQ_INT(4, count_metrics(set)); metric_family_free(fam); + /* adding a the same metric with a different time stamp should work. */ + CHECK_NOT_NULL(fam = make_metric_family(1, 1, 1, 2)); + CHECK_ZERO(resource_metrics_add(&set, fam)); + /* reuses existing resource */ + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(5, count_metrics(set)); + /* adding the same metric twice should return EEXIST. */ + EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + EXPECT_EQ_INT(2, set.num); + EXPECT_EQ_INT(5, count_metrics(set)); + metric_family_free(fam); + resource_metrics_reset(&set); /* adding 1000 distinct metrics. */ @@ -113,8 +127,8 @@ DEF_TEST(resource_metrics_add) { for (int j = 0; j < 10; j++) { for (int k = 0; k < 10; k++) { /* add metrics in "random" order */ - CHECK_NOT_NULL( - fam = make_metric_family((i * 7) % 10, (j * 7) % 10, (k * 7) % 10)); + CHECK_NOT_NULL(fam = make_metric_family((i * 7) % 10, (j * 7) % 10, + (k * 7) % 10, 1)); EXPECT_EQ_INT(0, resource_metrics_add(&set, fam)); want_metrics_count++; EXPECT_EQ_INT(want_metrics_count, count_metrics(set)); From 4551377f70b790585c9d224a6549c0472a18e0a1 Mon Sep 17 00:00:00 2001 From: Florian Forster Date: Sun, 17 Dec 2023 09:45:11 +0100 Subject: [PATCH 8/8] resource_metrics: Skip duplicate metrics instead of erroring out. The semantics have been changed to simply ignore metrics that are already in the set. The previous semantic was optimized for a "add and if that fails flush and try again" plugin behavior. With the support for multiple instances of the same metric (at different times), there no longer is a need to ensure metrics in the set are unique. --- src/utils/resource_metrics/resource_metrics.c | 32 ++++++++++++------- src/utils/resource_metrics/resource_metrics.h | 10 ++++-- .../resource_metrics/resource_metrics_test.c | 22 ++++++------- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/src/utils/resource_metrics/resource_metrics.c b/src/utils/resource_metrics/resource_metrics.c index b5eca7f831..8e49bcc9dd 100644 --- a/src/utils/resource_metrics/resource_metrics.c +++ b/src/utils/resource_metrics/resource_metrics.c @@ -167,32 +167,42 @@ static bool metric_exists(metric_family_t const *fam, metric_t const *m) { } static int insert_metrics(metric_family_t *fam, metric_list_t metrics) { + int skipped = 0; for (size_t i = 0; i < metrics.num; i++) { - if (metric_exists(fam, metrics.ptr + i)) { - return EEXIST; + metric_t const *m = metrics.ptr + i; + + if (metric_exists(fam, m)) { +#if COLLECT_DEBUG + strbuf_t buf = STRBUF_CREATE; + metric_identity(&buf, m); + DEBUG("resource_metrics: Skipping duplicate of metric %s", buf.ptr); + STRBUF_DESTROY(buf); +#endif + skipped++; + continue; } - } - int last_err = 0; - for (size_t i = 0; i < metrics.num; i++) { - int status = metric_family_metric_append(fam, metrics.ptr[i]); + int status = metric_family_metric_append(fam, *m); if (status != 0) { ERROR("resource_metrics: metric_family_metric_append failed: %s", STRERROR(status)); /* DO NOT RETURN: the metric list may be unsorted */ - last_err = status; + skipped++; } } - qsort(fam->metric.ptr, fam->metric.num, sizeof(*fam->metric.ptr), - (void *)compare_metrics); - return last_err; + if (((size_t)skipped) != metrics.num) { + qsort(fam->metric.ptr, fam->metric.num, sizeof(*fam->metric.ptr), + (void *)compare_metrics); + } + + return skipped; } int resource_metrics_add(resource_metrics_set_t *set, metric_family_t const *fam) { if (set == NULL || fam == NULL) { - return EINVAL; + return -1; } resource_metrics_t *rm = lookup_or_insert_resource(set, fam->resource); diff --git a/src/utils/resource_metrics/resource_metrics.h b/src/utils/resource_metrics/resource_metrics.h index ffda8e14c5..f52468d44d 100644 --- a/src/utils/resource_metrics/resource_metrics.h +++ b/src/utils/resource_metrics/resource_metrics.h @@ -47,8 +47,14 @@ typedef struct { } resource_metrics_set_t; /* resource_metrics_add copies a metric family to the resource metrics set. - * If any metric within the metric family is already part of the resource - * metrics set, the function will return EEXIST and rm remains unmodified. */ + * Identical metrics are skipped and not added to the set. Metrics are + * identical, if their resource attributes, metric family name, metric labels, + * and time stamp are equal. + * Returns the number of metrics that were skipped or -1 on error. That means + * that zero indicates complete success, a positive number indicates partial + * success, and a negative number indicates an error condition. The number of + * skipped entries may be equal to the total number of metrics provided; this is + * not indicated as an error. */ int resource_metrics_add(resource_metrics_set_t *rm, metric_family_t const *fam); diff --git a/src/utils/resource_metrics/resource_metrics_test.c b/src/utils/resource_metrics/resource_metrics_test.c index 859b660f7c..8507b198d3 100644 --- a/src/utils/resource_metrics/resource_metrics_test.c +++ b/src/utils/resource_metrics/resource_metrics_test.c @@ -65,20 +65,20 @@ DEF_TEST(resource_metrics_add) { CHECK_ZERO(resource_metrics_add(&set, fam)); EXPECT_EQ_INT(1, set.num); EXPECT_EQ_INT(1, count_metrics(set)); - /* adding the same familiy twice should return EEXIST. */ - EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + /* adding the same metric twice should return one skipped metric. */ + EXPECT_EQ_INT(1, resource_metrics_add(&set, fam)); EXPECT_EQ_INT(1, set.num); EXPECT_EQ_INT(1, count_metrics(set)); metric_family_free(fam); - /* adding the same metric (but with a different resource attribute) should + /* adding the same metric family with different resource attributes should * succeed. */ CHECK_NOT_NULL(fam = make_metric_family(2, 1, 1, 1)); CHECK_ZERO(resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(2, count_metrics(set)); - /* adding the same familiy twice should return EEXIST. */ - EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + /* adding the same metric twice should return one skipped metric. */ + EXPECT_EQ_INT(1, resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(2, count_metrics(set)); metric_family_free(fam); @@ -89,8 +89,8 @@ DEF_TEST(resource_metrics_add) { /* reuses existing resource */ EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(3, count_metrics(set)); - /* adding the same familiy twice should return EEXIST. */ - EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + /* adding the same metric twice should return one skipped metric. */ + EXPECT_EQ_INT(1, resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(3, count_metrics(set)); metric_family_free(fam); @@ -101,8 +101,8 @@ DEF_TEST(resource_metrics_add) { /* reuses existing resource */ EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(4, count_metrics(set)); - /* adding the same familiy twice should return EEXIST. */ - EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + /* adding the same metric twice should return one skipped metric. */ + EXPECT_EQ_INT(1, resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(4, count_metrics(set)); metric_family_free(fam); @@ -113,8 +113,8 @@ DEF_TEST(resource_metrics_add) { /* reuses existing resource */ EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(5, count_metrics(set)); - /* adding the same metric twice should return EEXIST. */ - EXPECT_EQ_INT(EEXIST, resource_metrics_add(&set, fam)); + /* adding the same metric twice should return one skipped metric. */ + EXPECT_EQ_INT(1, resource_metrics_add(&set, fam)); EXPECT_EQ_INT(2, set.num); EXPECT_EQ_INT(5, count_metrics(set)); metric_family_free(fam);