On Tue, Dec 06, 2022 at 05:12:59PM +0100, Claudio Jeker wrote:
> When implementing this initially I thought that suffixes are merely a
> suggestion but now I realize that the spec is a more strict about them.
> So introduce a list of reserved suffixes and bail out if a metric is added
> that uses such a suffix. Also fail if a metric name is reused.
>
> The output code change is currently good enough. Since there is no support
> for the more complex types like histogram and the _created value for
> counters is deliberatly not implemented.
I'm ok with this.
A suggestion below (and a couple tiny nits).
> Index: ometric.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/ometric.c,v
> retrieving revision 1.6
> diff -u -p -r1.6 ometric.c
> --- ometric.c 6 Dec 2022 11:27:58 -0000 1.6
> +++ ometric.c 6 Dec 2022 15:51:33 -0000
> @@ -71,6 +71,51 @@ struct ometric {
>
> STAILQ_HEAD(, ometric) ometrics = STAILQ_HEAD_INITIALIZER(ometrics);
>
> +static const struct {
> + const char *suffix;
> + size_t suffix_len;
> +} suffixes[] = {
> + { "_total", sizeof("_total") - 1 },
> + { "_created", sizeof("_created") - 1 },
> + { "_count", sizeof("_count") - 1 },
> + { "_sum", sizeof("_sum") - 1 },
> + { "_bucket", sizeof("_bucket") - 1 },
> + { "_gcount", sizeof("_gcount") - 1 },
> + { "_gsum", sizeof("_gsum") - 1 },
> + { "_info", sizeof("_info") - 1 },
> +};
> +
> +/*
> + * Return true if name has one of the above suffixes.
> + */
> +static int
> +strsuffix(const char *name)
> +{
> + size_t i, len;
> +
> + len = strlen(name);
I was wondering if it's worth doing a strrchr up front to simplify the rest:
suffix = strrchr(name, '_');
if (suffix == NULL)
return 0;
len = strlen(suffix);
for (i = 0; i < sizeof(suffixes) / sizeof(suffixes[0]); i++) {
if (len != suffixes[i].suffix_len)
continue;
if (strcmp(suffix, suffixes[i].suffix) == 0)
return 1;
}
return 0;
Once we get there, we could also drop len and .suffix_len, use a plain
array of strings, and do this:
const char *suffixes[] = { "_total", "_created", ... };
...
suffix = strrchr(name, '_');
if (suffix == NULL)
return 0;
for (i = 0; i < sizeof(suffixes) / sizeof(suffixes[0]); i++) {
if (strcmp(suffix, suffixes[i]) == 0)
return 1;
}
return 0;
...
> + for (i = 0; i < sizeof(suffixes) / sizeof(suffixes[0]); i++) {
> + if (len < suffixes[i].suffix_len)
> + continue;
> + if (strcmp(name + len - suffixes[i].suffix_len,
> + suffixes[i].suffix) == 0)
> + return 1;
> + }
> + return 0;
Trailing tab after ;
> +}
> +
> +static void
> +ometric_check(const char *name)
> +{
> + struct ometric *om;
> +
> + if (strsuffix(name))
> + errx(1, "reserved name suffix used: %s", name);
> + STAILQ_FOREACH(om, &ometrics, entry)
> + if (strcmp(name, om->name) == 0)
> + errx(1, "duplicate name: %s", name);
> +}
> +
> /*
> * Allocate and return new ometric. The name and help string need to remain
> * valid until the ometric is freed. Normally constant strings should be
> used.
> @@ -80,6 +125,8 @@ ometric_new(enum ometric_type type, cons
> {
> struct ometric *om;
>
> + ometric_check(name);
> +
> if ((om = calloc(1, sizeof(*om))) == NULL)
> err(1, NULL);
>
> @@ -88,6 +135,7 @@ ometric_new(enum ometric_type type, cons
> om->type = type;
> STAILQ_INIT(&om->vals);
>
> +
I'd not add an extra empty line.
> STAILQ_INSERT_TAIL(&ometrics, om, entry);
>
> return om;
> @@ -104,6 +152,8 @@ ometric_new_state(const char * const *st
> {
> struct ometric *om;
>
> + ometric_check(name);
> +
> if ((om = calloc(1, sizeof(*om))) == NULL)
> err(1, NULL);
>
> @@ -284,6 +334,25 @@ ometric_output_value(FILE *out, const st
> return -1;
> }
>
> +static int
> +ometric_output_name(FILE *out, const struct ometric *om)
> +{
> + const char *suffix;
> +
> + switch (om->type) {
> + case OMT_COUNTER:
> + suffix = "_total";
> + break;
> + case OMT_INFO:
> + suffix = "_info";
> + break;
> + default:
> + suffix = "";
> + break;
> + }
> + return fprintf(out, "%s%s", om->name, suffix);
> +}
> +
> /*
> * Output all metric values with TYPE and optional HELP strings.
> */
> @@ -304,7 +373,7 @@ ometric_output_all(FILE *out)
> return -1;
>
> STAILQ_FOREACH(ov, &om->vals, entry) {
> - if (fprintf(out, "%s", om->name) < 0)
> + if (ometric_output_name(out, om) < 0)
> return -1;
> if (ometric_output_labels(out, ov->labels) < 0)
> return -1;
> Index: output_ometric.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpctl/output_ometric.c,v
> retrieving revision 1.7
> diff -u -p -r1.7 output_ometric.c
> --- output_ometric.c 6 Dec 2022 11:27:58 -0000 1.7
> +++ output_ometric.c 6 Dec 2022 15:52:11 -0000
> @@ -58,7 +58,7 @@ ometric_head(struct parse_result *arg)
> char hostname[HOST_NAME_MAX + 1];
> char *domainname;
>
> - bgpd_info = ometric_new(OMT_INFO, "bgpd_info", "bgpd information");
> + bgpd_info = ometric_new(OMT_INFO, "bgpd", "bgpd information");
> bgpd_scrape_time = ometric_new(OMT_GAUGE, "bgpd_scrape_seconds",
> "bgpd scrape time in seconds");
>
> @@ -82,7 +82,7 @@ ometric_head(struct parse_result *arg)
> * per neighbor stats: attrs will be remote_as, remote_addr,
> * description and group
> */
> - peer_info = ometric_new(OMT_INFO, "bgpd_peer_info",
> + peer_info = ometric_new(OMT_INFO, "bgpd_peer",
> "peer information");
> peer_state = ometric_new_state(statenames,
> sizeof(statenames) / sizeof(statenames[0]), "bgpd_peer_state",
> @@ -105,65 +105,65 @@ ometric_head(struct parse_result *arg)
> "number of prefixes received from peer");
>
> peer_message_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_message_transmit_total",
> + "bgpd_peer_message_transmit",
> "per message type count of transmitted messages");
> peer_message_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_message_receive_total",
> + "bgpd_peer_message_receive",
> "per message type count of received messages");
>
> peer_update_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_update_transmit_total",
> + "bgpd_peer_update_transmit",
> "number of prefixes sent as update");
> peer_update_pending = ometric_new(OMT_COUNTER,
> - "bgpd_peer_update_pending_total",
> + "bgpd_peer_update_pending",
> "number of pending update prefixes");
> peer_update_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_update_receive_total",
> + "bgpd_peer_update_receive",
> "number of prefixes received as update");
>
> peer_withdraw_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_withdraw_transmit_total",
> + "bgpd_peer_withdraw_transmit",
> "number of withdrawn prefixes sent to peer");
> peer_withdraw_pending = ometric_new(OMT_COUNTER,
> - "bgpd_peer_withdraw_pending_total",
> + "bgpd_peer_withdraw_pending",
> "number of pending withdrawn prefixes");
> peer_withdraw_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_withdraw_receive_total",
> + "bgpd_peer_withdraw_receive",
> "number of withdrawn prefixes received from peer");
>
> peer_rr_req_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_req_transmit_total",
> + "bgpd_peer_route_refresh_req_transmit",
> "number of route-refresh request transmitted to peer");
> peer_rr_req_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_req_receive_total",
> + "bgpd_peer_route_refresh_req_receive",
> "number of route-refresh request received from peer");
> peer_rr_borr_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_borr_transmit_total",
> + "bgpd_peer_route_refresh_borr_transmit",
> "number of ext. route-refresh BORR messages transmitted to peer");
> peer_rr_borr_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_borr_receive_total",
> + "bgpd_peer_route_refresh_borr_receive",
> "number of ext. route-refresh BORR messages received from peer");
> peer_rr_eorr_transmit = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_eorr_transmit_total",
> + "bgpd_peer_route_refresh_eorr_transmit",
> "number of ext. route-refresh EORR messages transmitted to peer");
> peer_rr_eorr_receive = ometric_new(OMT_COUNTER,
> - "bgpd_peer_route_refresh_eorr_receive_total",
> + "bgpd_peer_route_refresh_eorr_receive",
> "number of ext. route-refresh EORR messages received from peer");
>
> /* RDE memory statistics */
> rde_mem_size = ometric_new(OMT_GAUGE,
> "bgpd_rde_memory_usage_bytes", "memory usage in bytes");
> rde_mem_count = ometric_new(OMT_GAUGE,
> - "bgpd_rde_memory_count", "number of object in use");
> + "bgpd_rde_memory_usage_objects", "number of object in use");
> rde_mem_ref_count = ometric_new(OMT_GAUGE,
> - "bgpd_rde_memory_reference_count", "number of references held");
> + "bgpd_rde_memory_usage_references", "number of references held");
>
> rde_set_size = ometric_new(OMT_GAUGE,
> "bgpd_rde_set_usage_bytes", "memory usage of set in bytes");
> rde_set_count = ometric_new(OMT_GAUGE,
> - "bgpd_rde_set_count", "number of object in set");
> + "bgpd_rde_set_usage_objects", "number of object in set");
> rde_table_count = ometric_new(OMT_GAUGE,
> - "bgpd_rde_table_count", "number of as_set tables");
> + "bgpd_rde_set_usage_tables", "number of as_set tables");
> }
>
> static void
>
