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: expose secpoll status #7197

Merged
merged 2 commits into from Nov 20, 2018

Conversation

Projects
None yet
3 participants
@pieterlexis
Copy link
Member

commented Nov 14, 2018

Short description

This PR adds the security status to the SNMP, carbon and prometheus statistics. It also adds a showSecurityStatus function.

Closes #7194

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@pieterlexis pieterlexis requested a review from rgacogne Nov 14, 2018

@Habbie

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

This pull request introduces 1 alert when merging 9b52086 into aec3f40 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

Comment posted by LGTM.com

@pieterlexis

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2018

I'm somewhat sure that lgtm message is not true. @rgacogne ?

@pieterlexis pieterlexis force-pushed the pieterlexis:dnsdist-expose-secpoll branch from 9b52086 to 372ab52 Nov 14, 2018

@Habbie

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

I'm somewhat sure that lgtm message is not true. @rgacogne ?

I'd say so. The extremely similar code in showVersion right above has the same confused reporting, by the way.

@Habbie

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

This pull request introduces 1 alert when merging 372ab52 into aec3f40 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

Comment posted by LGTM.com

Show resolved Hide resolved pdns/dnsdist-snmp.cc Outdated
Show resolved Hide resolved pdns/dnsdist-snmp.cc Outdated
Show resolved Hide resolved pdns/dnsdistdist/DNSDIST-MIB.txt Outdated
@rgacogne

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

I'd say so. The extremely similar code in showVersion right above has the same confused reporting, by the way.

It looks like lgtm.com is confused indeed.

pieterlexis added some commits Nov 14, 2018

dnsdist: expose secpoll status in metrics
The status is now exposed in SNMP, carbon and prometheus.

@pieterlexis pieterlexis force-pushed the pieterlexis:dnsdist-expose-secpoll branch from 372ab52 to eb0ca46 Nov 15, 2018

@Habbie

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

This pull request introduces 1 alert when merging eb0ca46 into aec3f40 - view on LGTM.com

new alerts:

  • 1 for Missing return statement

Comment posted by LGTM.com

@rgacogne
Copy link
Member

left a comment

One nit, looks very good otherwise!


double latencyAvg100{0}, latencyAvg1000{0}, latencyAvg10000{0}, latencyAvg1000000{0};
typedef std::function<uint64_t(const std::string&)> statfunction_t;
typedef boost::variant<stat_t*, double*, statfunction_t> entry_t;
typedef boost::variant<stat_t*, double*, statfunction_t, uint64_t*> entry_t;

This comment has been minimized.

Copy link
@rgacogne

rgacogne Nov 15, 2018

Member

I believe this change is not needed anymore?

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Nov 15, 2018

@pieterlexis pieterlexis merged commit 5d2b9cb into PowerDNS:master Nov 20, 2018

4 checks passed

LGTM analysis: C/C++ 1 new alert
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No alert changes
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pieterlexis pieterlexis deleted the pieterlexis:dnsdist-expose-secpoll branch Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.