On Wed, Oct 12, 2022 at 12:12:25PM +0200, Theo Buehler wrote:
> On Fri, Oct 07, 2022 at 12:37:10PM +0200, Claudio Jeker wrote:
> > This diff adds `bgpctl show metric` which is a command that dumps some
> > stats out in openmetric format. This format can be ingested by e.g.
> > prometheus and used for monitoring.
> >
> > The openmetric handling is in ometric.[ch]. It is fairly basic and not
> > intended for long running processes. There is a struct ometric (which is
> > one individual metric point). This metric point can have many different
> > values with each value including an optional set of labels. Since the
> > labels are used over and over again, I used a refcount on them.
> > Also since most strings used in these functions are string literals I also
> > don't copy them. Only the values of labels are copied since those are
> > for example per peer.
> >
> > Using a small extra diff in bgplgd I can export the metrics into
> > prometheus and visualize them with grafana.
> >
> > Consider this an MVP that can be extended with all the infos we want.
>
> This looks pretty good to me. I like the approach and I couldn't spot
> anything really wrong with it.
>
> Some very minor comments inline for your consideration.
>
Thanks, some comments below.
> > +struct ometric *
> > +ometric_new_state(const char * const *states, size_t statecnt, const char
> > *name,
> > + const char *help)
> > +{
> > + struct ometric *om;
>
> Matter of taste, but I'd probably have made an ometric_new_internal()
> that sets all fields, so ometric_new() and ometric_new_state() could be
> thin wrappers. This would also avoid leaving stateset and setsize
> uninitialized in ometric_new() (which I find a bit nasty).
>
> Or use calloc().
I switched to use calloc() in both cases.
Not totally sold on ometric_new_state(), the statesets are a pain to work
with and are highly inefficent (they turn into a metric point per state).
> > +void
> > +ometric_set_state(struct ometric *om, const char *state, struct olabels
> > *ol)
> > +{
> > + struct olabels *extra;
> > + size_t i;
> > + int val;
> > +
> > + if (om->type != OMT_STATESET)
> > + errx(1, "%s incorrect ometric type", __func__);
> > +
> > + for (i = 0; i < om->setsize; i++) {
> > + if (strcasecmp(state, om->stateset[i]) == 0)
> > + val = 1;
> > + else
> > + val = 0;
>
> could simplify this to
>
> val = strcasecmp(state, om->stateset[i]) == 0;
>
> but I'm not sure if this is more readable
Not sure either. I prefer the explicit version. So I left the code as is.
> > +struct ometric *bgpd_info, *bgpd_scrape_time;
> > +struct ometric *peer_info, *peer_state, *peer_up_time, *peer_down_time,
> > + *peer_last_read, *peer_last_write;
> > +struct ometric *peer_prefixes_tranmit, *peer_prefixes_receive;
>
> typo: peer_prefixes_transmit
Fixed including all the other minor changes.
--
:wq Claudio
