Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dnsdist: unify global stats accounting #6289

Merged
merged 6 commits into from Mar 6, 2018
Merged

Conversation

@zeha
Copy link
Collaborator

@zeha zeha commented Feb 20, 2018

Short description

Clean up some statistics accounting which was handled inconsistently before:

  • Before, UDP selfAnswered was increased even if no response was sent, while this isn't done for TCP or UDP/TCP cacheHits. Now it is only increased if a response goes out.
  • Global latency stats were not updated for TCP at all - to avoid changing current behaviour, this is dropped
  • Treat selfAnswered, cacheHits, and noPolicy the same (=latency 0) for global latency stats, both UDP and TCP

Additional stats issues that this PR does not fix:

  • UDP and TCP update aclDrops, nonCompliantQueries, queries at different times
  • nonCompliantQueries is increased either before or after queries is increased

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
@zeha zeha added this to the dnsdist-1.3.0 milestone Feb 20, 2018
@zeha zeha changed the title dnsdist: unify stats accounting dnsdist: unify global stats accounting Feb 20, 2018
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Feb 20, 2018

Thanks! Code looks good, although I'm not sure we want to take the TCP response time into account in our latency calculations. It is my understanding that by design we only care about the UDP response time, especially because we use it for our policies, but I might be mistaken.

@zeha
Copy link
Collaborator Author

@zeha zeha commented Feb 20, 2018

I think latency is only used by the leastOutstanding policy, which looks at DownstreamState latencyUsec. This PR does not touch the DSS latencyUsec for TCP. (This could have been clearer...)

@zeha
Copy link
Collaborator Author

@zeha zeha commented Feb 20, 2018

OTOH: I could add extra timekeeping for TCP, just before writing the response out. Then we could use that and update DSS latencyUsec too. Y/N/Scary?

@zeha zeha force-pushed the zeha:dnsdist-stats branch from b027f39 to 1216612 Mar 6, 2018
@zeha
Copy link
Collaborator Author

@zeha zeha commented Mar 6, 2018

I've dropped the TCP latency updating for now, can discuss this later.

@rgacogne rgacogne merged commit cb02f76 into PowerDNS:master Mar 6, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants