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: implemented query counting #4175

Merged
merged 1 commit into from Aug 23, 2016

Conversation

Projects
None yet
3 participants
@skoef
Contributor

skoef commented Jul 14, 2016

I like the carbonServer functionality, but want to know more about the queries (or in my specific case: domains) that are served by dnsdist. So I implemented query counting, a per-record counter of the amount of requests for a specific record. This then is included in the data sent to carbon and can be adjusted/filtered by setting a Lua function with setQueryCountFilter()

g_lua.writeFunction("clearQueryLog", []() {
unsigned int size = g_qlog.records.size();
g_qlog.records.clear();

This comment has been minimized.

@rgacogne

rgacogne Jul 18, 2016

Member

It looks like this code need to acquire a write lock.

g_lua.writeFunction("getQueryLog", [](boost::optional<unsigned int> optMax) {
setLuaNoSideEffect();
boost::format fmt("query logging is currently: %s (%d records in buffer)\n");
g_outputBuffer = (fmt % (g_qlog.enabled ? "enabled" : "disabled") % g_qlog.records.size()).str();

This comment has been minimized.

@rgacogne

rgacogne Jul 18, 2016

Member

And probably a read lock there.

@skoef skoef changed the title from Implemented query logging to dnsdist: implemented query counting Aug 17, 2016

WriteLock wl(&g_qcount.queryLock);
QueryCountRecords::iterator it;
std::string qname;
for(it=g_qcount.records.begin(); it!=g_qcount.records.end(); ++it) {

This comment has been minimized.

@cmouse

cmouse Aug 17, 2016

Contributor

maybe use for(auto qname: g_qcount.records) here?

QueryCountRecords::iterator it;
unsigned int max = optMax ? *optMax : 10;
unsigned int index{1};
for(it = g_qcount.records.begin(); it != g_qcount.records.end(); ++it, ++index) {

This comment has been minimized.

@cmouse

cmouse Aug 17, 2016

Contributor

for(it = g_qcount.records.begin(); it != g_qcount.records.end() && index <= max; ++it, ++index);

This comment has been minimized.

@cmouse

cmouse Aug 17, 2016

Contributor

or just it = g_qcount.records.begin() + std::min(g_qcount.records.size(), max);

the amount of queries by using `setQueryCount(true)`. With query counting enabled,
`dnsdist` will increase a counter for every unique record or the behaviour you define
in a custom Lua function by setting `setQueryCountFilter(func)`. This filter can decide
whether to keep count on a query at all or rewrite the queried record.

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

Perhaps clarify that only the counter key is rewritten, not the query. It should make sense of course, but I am worried about someone skimming over the README looking how to rewrite queries.

return false, ""
end
-- rewrite these queries

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

Same thing here, it might be useful to clarify that the counting key is rewritten, not the query.

Valid return values for `QueryCountFilter` functions are:
* `true`: count the specified query
* `false`: discard the query

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

Perhaps replace discard the query by don't count the query?

@@ -748,6 +749,22 @@ bool processQuery(LocalStateHolder<NetmaskTree<DynBlock> >& localDynNMGBlock,
g_rings.queryRing.push_back({now,*dq.remote,*dq.qname,dq.len,dq.qtype,*dq.dh});
}
if(g_qcount.enabled) {
WriteLock wl(&g_qcount.queryLock);

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

I don't think you need to acquire a WriteLock until after if (countQuery) { evaluates to true below

string qname = (*dq.qname).toString(".", false);
bool countQuery{true};
if(g_qcount.filter) {
std::tie (countQuery, qname) = g_qcount.filter(dq);

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

I think you should acquire the Lua mutex here with something like std::lock_guard<std::mutex> lock(g_luamutex);

@@ -748,6 +749,22 @@ bool processQuery(LocalStateHolder<NetmaskTree<DynBlock> >& localDynNMGBlock,
g_rings.queryRing.push_back({now,*dq.remote,*dq.qname,dq.len,dq.qtype,*dq.dh});
}
if(g_qcount.enabled) {
WriteLock wl(&g_qcount.queryLock);
string qname = (*dq.qname).toString(".", false);

This comment has been minimized.

@rgacogne

rgacogne Aug 23, 2016

Member

Just a nit, but for a better readability we have toStringNoDot() that does toString(".", false)

@rgacogne

This comment has been minimized.

Member

rgacogne commented Aug 23, 2016

LGTM once travis is happy. Thank you!

@skoef skoef force-pushed the skoef:queryLogging branch from 52e70bc to 786e4d8 Aug 23, 2016

@rgacogne rgacogne merged commit 55c7d8e into PowerDNS:master Aug 23, 2016

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