SuperQ / chrony_exporter

Exporter for Chrony NTP
Apache License 2.0
49 stars 13 forks source link

RefID garbled #7

Closed hhoffstaette closed 2 years ago

hhoffstaette commented 2 years ago

Thanks so much for this project! I know it's early days but I found something odd:

Chrony says:

holger>chronyc tracking 
Reference ID    : C0A864C8 (bifrost.applied-asynchrony.com)

The exporter says:

holger>curl -s http://localhost:9123/metrics | grep refid                             
chrony_tracking_info{tracking_address="192.168.100.200",tracking_refid="À¨dÈ"} 1

Running the prebuilt 0.2.0 on x64 against chrony 4.2.

SuperQ commented 2 years ago

Yea, odd. This seems to be a bug in the upstream library. Something is wrong with the RefidToString() function.

SuperQ commented 2 years ago

Ok, so it looks like the refid is a simple fix by using fmt.Sprintf("%X", ...). But in order to print the "name", we need to implement Chrony's name formatting:

format_name(char *buf, int size, int trunc_dns, int ref, uint32_t ref_id,
            int source, IPAddr *ip_addr)
{
  if (ref) {
    snprintf(buf, size, "%s", UTI_RefidToString(ref_id));
  } else if (source && source_names) {
    if (!get_source_name(ip_addr, buf, size))
      snprintf(buf, size, "?");
  } else if (no_dns || csv_mode) {
    snprintf(buf, size, "%s", UTI_IPToString(ip_addr));
  } else {
    DNS_IPAddress2Name(ip_addr, buf, size);
    if (trunc_dns > 0 && strlen(buf) > trunc_dns) {
      buf[trunc_dns - 1] = '>';
      buf[trunc_dns] = '\0';
    }
  }
}
SuperQ commented 2 years ago

Let me know if the fix works for you, I can then cut a release.

hhoffstaette commented 2 years ago

Better than before, but I'm not sure about the multiple host names:

holger>curl -s http://localhost:9123/metrics | grep refid
chrony_tracking_info{tracking_address="192.168.100.200",tracking_name="bifrost.applied-asynchrony.com.,bifrost",tracking_refid="C0A864C8"} 1

It seems to resolve all names..?

SuperQ commented 2 years ago

Yes, Go net.LookupAddr() returns a list of resolved names. Chrony seems to use the obsolete gethostbyaddr() method.

I guess we could do something like only expose the first match returned by net.LookupAddr().

hhoffstaette commented 2 years ago

The host is one of the few well-defined machines I have in /etc/hosts, and as expected it had both FQDN and short name in there (which has always worked as expected). If you can easily use only the first name when multiple names are returned that would be great. Also not sure why it insists on adding the terminating dot, but whatever. With only the FQDN in hosts it looks right:

holger>curl -s http://localhost:9123/metrics | grep refid          
chrony_tracking_info{tracking_address="192.168.100.200",tracking_name="bifrost.applied-asynchrony.com.",tracking_refid="C0A864C8"} 1

I think the real win here was that the refid is now correct, so good job ;-)

SuperQ commented 2 years ago

The trailing dot is to specify the result is an FQDN. I guess I need to look at the Go resolver docs more closely.