[Dnsmasq-discuss] ubus and metrics

[Julian Kornberger]

On 20.04.2018 23:04, Simon Kelley wrote:
> 1) Conditional compilation.
HAVE_METRICS has been removed.

> 2) In config.h, you document HAVE_UBUS, but don't include it in the
> features string, whilst you include HAVE_METRIC in the features string,
> but don't document it.
Has been added.

> 3) The documentation for the GetMetrics DBus method is almost
> non-existent. The documentation for the UBus interface is entirely
> non-existent.
I asked the original author to write some documentation.

> 4) The list of metrics is in metrics.h and the list of their names is in
> metrics.c and the two have to be kept in sync. Can this be done some way
> that at least keeps the two in the same source file, and preferably
> allows the addition of a new metric by adding one line?
I don't have any idea how to achieve this. I added a hint to the enum.

> 5) What is the motivation for the metrics you're collecting? Is that
> actually useful information? What uses it?
Sometimes there are clients that behave in a strange way. Like acquiring 
a same lease every second and ignoring the reply. Metrics help to debug 
these kind of problems. If you observe a specific message counter to 
increase rapidly (like NAKs), then something might be wrong in your network.

You get a graph like this:
https://grafana.ffhb.de/d/YYHhL-Wmz/dhcp?orgId=1&from=1524212492387&to=1524253637769

> 6)
> Some files have
>
> METRICS_INCREMENT(<metric>);
>
> and some have
>
> #ifdef HAVE_METRICS
>       metrics[<metric>]++;
> #endif
>
> Any good reason for that?
METRICS_INCREMENT is now used everwhere.

> 7) Apart from reading metrics, there seems to be code to broadcast some
> events over UBus. But no justification for doing this and no
> documentation of what is done.
>
> 8) The convention in dnsmasq is that global data structures are part of
> the "daemon" structure, so
>
> metrics[<metric>]
>
> should be
>
> daemon->metrics[metric]
>
> and the array should be allocated and initialised appropriately.
Has been changed

> 9) Dnsmasq already collects some metrics about DNS forwarder operation,
> which are dumped to the log when the process receives SIGUSR1. Should
> this new metrics system embrace and extend the existing one, rather than
> ignoring it?
I removed the old metric variables and added new fields to the metrics 
struct.
ubus and dbus now also return the already existing metrics.

Regards
Julian Kornberger