That's great. Thank you. Can I make another request? It would be good (and consistent) to have a dnsmasq option for UBus analogous to --enable-dbus, called --enable-ubus so that dnsmasq doesn't try and connect to the Ubus unless that's explicitly requested, even if the code is compiled in. Like the DBUS version, it should raise a fatal error if the options is set but the UBus code is not compiled in.
Cheers, Simon. On 21/04/18 17:40, Julian Kornberger wrote: > 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