On Tue, Dec 06, 2022 at 06:21:31PM +0100, Theo Buehler wrote:
> 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;
> ...
>
Indeed much better. Updated diff below.
--
:wq Claudio
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 17:28:05 -0000
@@ -71,6 +71,41 @@ struct ometric {
STAILQ_HEAD(, ometric) ometrics = STAILQ_HEAD_INITIALIZER(ometrics);
+static const char *suffixes[] = { "_total", "_created", "_count",
+ "_sum", "_bucket", "_gcount", "_gsum", "_info",
+};
+
+/*
+ * Return true if name has one of the above suffixes.
+ */
+static int
+strsuffix(const char *name)
+{
+ const char *suffix;
+ size_t i;
+
+ 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;
+}
+
+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 +115,8 @@ ometric_new(enum ometric_type type, cons
{
struct ometric *om;
+ ometric_check(name);
+
if ((om = calloc(1, sizeof(*om))) == NULL)
err(1, NULL);
@@ -104,6 +141,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 +323,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 +362,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;
@@ -425,7 +483,7 @@ ometric_set_state(struct ometric *om, co
if (om->type != OMT_STATESET)
errx(1, "%s incorrect ometric type", __func__);
- for (i = 0; i < om->setsize; i++) {
+ for (i = 0; i < om->setsize; i++) {
if (strcasecmp(state, om->stateset[i]) == 0)
val = 1;
else
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
