[Dnsmasq-discuss] ubus and metrics

[Simon Kelley]

Comments, in no particular order.

1) Conditional compilation.

Every HAVE_<somefeature> doubles the number of combinations of
compile-time selections, and increases the chances that some set of
selections will fail to compile or be buggy. I'm not sure that
HAVE_METRICS passes the test for needing control at  compile-time: It's
not much extra code, and it doesn't add any build dependencies.

HAVE_UBUS is certainly needed, because of the dependency on libubus.

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.

3) The documentation for the GetMetrics DBus method is almost
non-existent. The documentation for the UBus interface is entirely
non-existent.

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?

5) What is the motivation for the metrics you're collecting? Is that
actually useful information? What uses it?

6)
Some files have

METRICS_INCREMENT(<metric>);

and some have

#ifdef HAVE_METRICS
     metrics[<metric>]++;
#endif

Any good reason for that?

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.

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?

Let's talk about what's actually needed here, and what is appropriate to
be carried upstream, rather than as a Lede patch, before going any further.


Cheers,

Simon.






On 17/04/18 12:14, Julian Kornberger wrote:
> Hi,
> 
> I used the ubus patch [1] of John Cripsin for OpenWRT and added some
> metrics that can be retreived via ubus and dbus. Please check out my
> changes [2] and tell me if anything has to be done to get it upstream.
> 
> [1]
> https://github.com/lede-project/source/blob/master/package/network/services/dnsmasq/patches/240-ubus.patch
> [2] https://github.com/digineo/dnsmasq/compare/metrics
> 
> Kind Regards,
> Julian Kornberger
> 
> _______________________________________________
> Dnsmasq-discuss mailing list
> Dnsmasq-discuss at lists.thekelleys.org.uk
> http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
>