duplicate metrics for "auth" system

[macb]

I would expect dns_error_count_total to be a subset of dns_request_duration_seconds_count as dns_request_duration_seconds_count should include the non-error requests in addition to those that happened to be errors. However, it seems like dns_error_count_total includes about twice as many requests.

ie I have
2.5m auth sum(skydns_skydns_dns_request_duration_seconds_count) by (system)
5m auth sum(skydns_skydns_dns_error_count_total) by (system)

Tracing through the code I was thinking maybe this may be involved as it seems to be the only place an error is recorded without a duration to go along side it and would be tagged as system=auth.

[miekg]

[ Quoting notifications@github.com in "[skynetservices/skydns] seemingly d..." ]

I would expect dns_error_count_total to be a subset of dns_request_duration_seconds_count as dns_request_duration_seconds_count should include the non-error requests in addition to those that happened to be errors. However, it seems like dns_error_count_total includes about twice as many requests.

ie I have
2.5m dns_request_duration_seconds_count
5m dns_error_count_total

Tracing through the code I was thinking maybe this may be involved as it seems to be the only place an error is recorded without a duration to go along side it.

Have you seen CoreDNS? (github.com/miekg/coredns) which uses a middleware
approach to build a cleaner SkyDNS (among other things).

CoreDNS is waaaaaay newer than SkyDNS, but the code is cleaner IMHO.

[macb]

Using skydns as part of the kube-dns addon so came across this (which arguably may be caused by some additional caching that pod does with dnsmasq). I'm not sure if they're planning on switching that piece out any time soon, maybe theres a plan for that!

[xvello]

We can replicate this with k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.10, that vendors in the affected code. So does #337

I think @macb 's investigation is right. From what I understand of the code, for each auth error, we:

• increase the counter via metrics.ReportErrorCount(m, metrics.Auth)
• call overflowOrTruncated(w, m, int(bufsize), metrics.Auth) that also calls ReportErrorCount

@miekg could you please confirm that is the case, or if we are missing something?

[miekg]

[ Quoting <notifications@github.com> in "Re: [skynetservices/skydns] duplica..." ]

We can replicate this with `k8s.gcr.io/k8s-dns-kube-dns-amd64:1.14.10`, that vendors in the affected code. So does #337 I think @macb 's investigation is right. From what I understand of the code, for each `auth` error, we: - increase the counter via [metrics.ReportErrorCount(m, metrics.Auth)](https://github.com/skynetservices/skydns/blob/15f42ac021b1f17a8b329f409539aa1624458da0/server/server.go#L253) - call [overflowOrTruncated(w, m, int(bufsize), metrics.Auth)](https://github.com/skynetservices/skydns/blob/15f42ac021b1f17a8b329f409539aa1624458da0/server/server.go#L284) that also [calls ReportErrorCount](https://github.com/skynetservices/skydns/blob/15f42ac021b1f17a8b329f409539aa1624458da0/server/server.go#L857) @miekg could you please confirm that is the case, or if we are missing something?

Sorry, I lack the time to also take care of SkyDNS (I unsubbed myself from the repo as well)

[xvello]

Thanks for your quick answer @miekg. Could you recommend someone else to investigate this issue with?

[miekg]

Not really. Skydns is basically unmaintained currently. Happy to click merge button at some point or add people who can do that

…

On Thu, 19 Jul 2018, 16:26 Xavier Vello, ***@***.***> wrote: Thanks for your quick answer @miekg <https://github.com/miekg>. Could you recommend someone else to investigate this issue with? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#298 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAVkW3MloQxXtctYAHhIJzz2KbJMPiaPks5uIKUIgaJpZM4KQe-Z> .

[bketelsen]

I'm happy to merge a fix for this if someone wants to send a PR.