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

Add Metric "server-up" Count to Carbon Reporting #6327

Merged
merged 8 commits into from Mar 20, 2018

Conversation

Projects
None yet
2 participants
@lowellmower

lowellmower commented Mar 8, 2018

Short description

This PR adds a metric (servers-up) to be pushed to carbon for servers in a pool which are in a state of UP. There already existed a metric for servers but that did not offer insight into the health of a ServerPool, rather was just a count for the total list of downstreams irrespective of their State. There did not appear to be an open issue/request for this.

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)

Proposed Metric:

dnsdist.carbonname1.main.pools._default_.servers-up 2 1520457187

Regression Test Output:

$ DNSDISTBIN=../pdns/dnsdistdist/dnsdist ./runtests test_Carbon.py
Generating a 2048 bit RSA private key
.......................................................................+++
...............................................+++
writing new private key to 'ca.key'
-----
Generating a 2048 bit RSA private key
.........................................................................+++
.......................................................+++
writing new private key to 'server.key'
-----
Signature ok
subject=/CN=tls.tests.dnsdist.org/OU=PowerDNS.com BV/C=NL
Getting CA Private Key
..
----------------------------------------------------------------------
Ran 2 tests in 12.345s

OK
@rgacogne

Thanks for this PR! A couple suggestions, and I think we can merge this quickly. As happy as I am to see trailing spaces go, please remove them in a separate commit whenever possible. It makes reviewing and especially looking for a specific changes later much more painful otherwise.

@@ -646,6 +646,16 @@ struct ServerPool
NumberedVector<shared_ptr<DownstreamState>> servers;
std::shared_ptr<DNSDistPacketCache> packetCache{nullptr};
std::shared_ptr<ServerPolicy> policy{nullptr};
int countServersUp() {
int upFound = 0;

This comment has been minimized.

@rgacogne

rgacogne Mar 9, 2018

Member

The type of upFound and the function return value should be unsigned, so size_t would be a good choice.

int countServersUp() {
int upFound = 0;
for(unsigned idx=0; idx<servers.size(); idx++) {

This comment has been minimized.

@rgacogne

rgacogne Mar 9, 2018

Member

Please use a c++11 loop whenever possible, as it's much more readable:

for (const auto& server : servers) {
  if (std::get<1>(server)->isUp() ) {
    upFound++;
  }
}
@@ -64,6 +77,30 @@ def startResponders(cls):
cls._CarbonResponder2.setDaemon(True)
cls._CarbonResponder2.start()
@classmethod

This comment has been minimized.

@rgacogne

rgacogne Mar 9, 2018

Member

This is a quite complicated way to have some servers up, perhaps you could just force them up in the configuration, so you don't have to actually start responder threads? The more responder threads we start the more issue we potentially have later with another test reusing the same port too quickly. Something like:

server = newServer(...)
server:setDown()
server = newServer(...)
server:setUp()
server = newServer(...)
server:setUp

@rgacogne rgacogne added this to the dnsdist-1.3.0 milestone Mar 9, 2018

@lowellmower

This comment has been minimized.

lowellmower commented Mar 9, 2018

@rgacogne - Sorry about including the whitespace in that commit, happy to reset and commit that change separately if you like. Good call on the configuration for the regression test, it was feeling like a little much and turns out it was.

Thanks for the quick feedback, let me know if there is something else you'd like me to address.

@rgacogne

Thanks for updating! A couple nits, looks good otherwise.

@@ -646,6 +646,16 @@ struct ServerPool
NumberedVector<shared_ptr<DownstreamState>> servers;
std::shared_ptr<DNSDistPacketCache> packetCache{nullptr};
std::shared_ptr<ServerPolicy> policy{nullptr};
int countServersUp() {

This comment has been minimized.

@rgacogne

rgacogne Mar 12, 2018

Member

The return type should be size_t, not int. You can also mark that method as const.

@@ -7,6 +7,10 @@
class TestCarbon(DNSDistTest):
_serverCount = 3
_serverUpCount = 2
_toResponderQueue1 = Queue()

This comment has been minimized.

@rgacogne

rgacogne Mar 12, 2018

Member

I think those two queues can go now?

This comment has been minimized.

@lowellmower

lowellmower Mar 12, 2018

Good catch - also removed _serverCount and _serverUpCount and just assert against 2 and 3 respectively.

@lowellmower

This comment has been minimized.

lowellmower commented Mar 12, 2018

@rgacogne - Thanks again for such quick feedback. Let me know if you see anything else.

@@ -646,6 +646,16 @@ struct ServerPool
NumberedVector<shared_ptr<DownstreamState>> servers;
std::shared_ptr<DNSDistPacketCache> packetCache{nullptr};
std::shared_ptr<ServerPolicy> policy{nullptr};
const size_t countServersUp() {

This comment has been minimized.

@rgacogne

rgacogne Mar 14, 2018

Member

This should be size_t countServersUp() const { instead, the current version is marking the return value const, which does not matter since it's neither a pointer nor a reference.

This comment has been minimized.

@lowellmower

lowellmower Mar 14, 2018

Ah, I misunderstood.

@lowellmower

This comment has been minimized.

lowellmower commented Mar 20, 2018

@rgacogne - thank you for the approval, is there anything else you need from me in order for this to be merged with master?

@rgacogne

This comment has been minimized.

Member

rgacogne commented Mar 20, 2018

Ah sorry, this PR was not on my radar because the tests were failing but it's not your doing. Merging now, thanks again!

@rgacogne rgacogne merged commit 8fa8a12 into PowerDNS:master Mar 20, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@rgacogne rgacogne referenced this pull request Mar 20, 2018

Merged

dnsdist: Ring buffers sharding #6191

4 of 6 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment