[Dnsmasq-discuss] ubus and metrics

[Julian Kornberger]

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

[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.

Post by Julian Kornberger
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
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Julian Kornberger]

Post by Simon Kelley
1) Conditional compilation.

HAVE_METRICS has been removed.

Post by Simon Kelley
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.

Post by Simon Kelley
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.

Post by Simon Kelley
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.

Post by Simon Kelley
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

Post by Simon Kelley
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.

Post by Simon Kelley
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

Post by Simon Kelley
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

[Simon Kelley]

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.

Post by Julian Kornberger

Post by Simon Kelley
1) Conditional compilation.

HAVE_METRICS has been removed.

Post by Simon Kelley
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.

Post by Simon Kelley
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.

Post by Simon Kelley
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.

Post by Simon Kelley
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.
https://grafana.ffhb.de/d/YYHhL-Wmz/dhcp?orgId=1&from=1524212492387&to=1524253637769

Post by Simon Kelley
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.

Post by Simon Kelley
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

Post by Simon Kelley
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

[Julian Kornberger]

_______________________________________________
Dnsmasq-discuss mailing list
Dnsmasq-***@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Simon Kelley]

Thanks for your comments.
I have added a `--enable-dbus` flag. It compiles but is not tested yet.

Thanks.

Where does the DBUS raise a fatal error if support is missing? I
couldn't find anything in the code.

src/dnsmasq.c:

if (option_bool(OPT_DBUS))
#ifdef HAVE_DBUS
{
char *err;
daemon->dbus = NULL;
daemon->watches = NULL;
if ((err = dbus_init()))
die(_("DBus error: %s"), err, EC_MISC);
}
#else
die(_("DBus not available: set HAVE_DBUS in src/config.h"), NULL,
EC_BADCONF);
#endif

- Is there any reason to not use the `enum` for definition lists like
`LOPT_*`?

No strong ones, just habit I guess.

- Is it planed to make the indentation consistent? Like only tabs or
only spaces.

I edit using emacs, and never see a problem. A massive edit would
generate a huge number of spurious changes in the git repository. I use
"git blame" quite often and don't want to find that it tells me half the
lines were last changed in the great re-tab.

Is there any advantage to doing it?


cheers,

Simon.

Regards,
Julian

Post by Simon Kelley
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.

[Kurt H Maier]

Post by Simon Kelley
I edit using emacs, and never see a problem. A massive edit would
generate a huge number of spurious changes in the git repository. I use
"git blame" quite often and don't want to find that it tells me half the
lines were last changed in the great re-tab.

git blame has a flag to ignore whitespace-only changes.

Post by Simon Kelley
Is there any advantage to doing it?

No.

khm

[Kevin Darbyshire-Bryant]

Post by Kurt H Maier

Post by Simon Kelley
I edit using emacs, and never see a problem. A massive edit would
generate a huge number of spurious changes in the git repository. I use
"git blame" quite often and don't want to find that it tells me half the
lines were last changed in the great re-tab.

git blame has a flag to ignore whitespace-only changes.

ooooh! I didn’t know that.

‘-w’ option to save anyone else digging through the manpage :-)

Cheers,

Kevin D-B

012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A

[Julian Kornberger]

Post by Simon Kelley

Where does the DBUS raise a fatal error if support is missing? I
couldn't find anything in the code.

Ok, I only searched in src/option.c

Post by Simon Kelley

- Is there any reason to not use the `enum` for definition lists like
`LOPT_*`?

No strong ones, just habit I guess.

Can this be refactored to simplify the code?

Post by Simon Kelley

- Is it planed to make the indentation consistent? Like only tabs or
only spaces.

I edit using emacs, and never see a problem. A massive edit would
generate a huge number of spurious changes in the git repository. I use
"git blame" quite often and don't want to find that it tells me half the
lines were last changed in the great re-tab.
Is there any advantage to doing it?

I always had to duplicate a line to have equal spaces/tabs. Indentation
looks weird when your editor does not know the width of a tab.
I would like to see a small style guide that has rules and examples for:

- identation (tabs or spaces)
- where to but curly braces (same line, next line? next line with
indentation?)
- adding whitespace after if/else/... and where else?
- never add ending whitspace to the end of the line

This makes the code much more readable for someone who does not have a
well configured editor.

Regards
Julian Kornberger

[Simon Kelley]

Post by Julian Kornberger

Post by Simon Kelley

Where does the DBUS raise a fatal error if support is missing? I
couldn't find anything in the code.

Ok, I only searched in src/option.c

Post by Simon Kelley

- Is there any reason to not use the `enum` for definition lists like
`LOPT_*`?

No strong ones, just habit I guess.

Can this be refactored to simplify the code?

It can be refactored, yes, but it probably won't simplify things.

Post by Julian Kornberger

Post by Simon Kelley

- Is it planed to make the indentation consistent? Like only tabs or
only spaces.

I edit using emacs, and never see a problem. A massive edit would
generate a huge number of spurious changes in the git repository. I use
"git blame" quite often and don't want to find that it tells me half the
lines were last changed in the great re-tab.
Is there any advantage to doing it?

I always had to duplicate a line to have equal spaces/tabs. Indentation
looks weird when your editor does not know the width of a tab.

The width of a tab is always 8, I think. That's the default for emacs.

Post by Julian Kornberger
- identation (tabs or spaces)

tabs are 8, but I can see the arguments for banning tabs and just using
space.

Post by Julian Kornberger
- where to but curly braces (same line, next line? next line with
indentation?)

The code is all pure gnu-style. Some hate it, but I like it :)

Post by Julian Kornberger
- adding whitespace after if/else/... and where else?You mean blank lines? I know what I do, but I'm not sure I could define it.
- never add ending whitspace to the end of the line

I've never understood why this is a problem, except maybe because of
noise in diffs.

Post by Julian Kornberger
This makes the code much more readable for someone who does not have a
well configured editor.

Set tab-width to 8 and all will be well.


Cheers,

Simon.

Post by Julian Kornberger
Regards
Julian Kornberger

[Simon Kelley]

Looking at the patch again, I think there's a problem with the original
Ubus patch. Because of where the ubus code is called in log_packet, Ubus
broadcasts will be suppressed by --quiet-dhcp. I doubt that's intended
behaviour.

It should at least be noted the both the UBus and metrics patches are
incomplete in DHCPv6 support.

It would be good to keep UBus and metrics are separate patches applied
sequentially.


Cheers,

Simon.