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
_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss
