Ftl: Pie chart percentages add up to more than 100 %

[KommX]

Versions

• Pi-hole version is v5.1.2 (Latest: v5.1.2)
• AdminLTE version is v5.1.1 (Latest: v5.1.1)
• FTL version is v5.2 (Latest: v5.2)

Platform

• OS and version: Raspbian GNU/Linux 10 (buster)
  
• Platform: Raspberry Pi

Expected behavior

The percentages in the mouseover tool tips of the pie charts should add up to (roughly) 100 %.

Actual behavior / bug

The percentages in the mouseover tool tips of the Queries answered by pie chart adds up to more than 100 %.

Steps to reproduce

Steps to reproduce the behavior:

1. Go to the main dashboard
2. Hover over the three sections blocklist, cache, localhost of the Queries answered by pie chart.
3. Calculate the sum of the three percentages.
4. The sum is bigger than 100 %, see screenshots, in my case 109

Debug Token

• URL: https://tricorder.pi-hole.net/89k3p725sy

Screenshots

[Chargnn]

Indeed, mine show 107.5%

[pralor-bot]

This issue has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/wrong-percent-calculation-in-pie-chart/44277/7

[Daxtorim]

This is really an issue with FTL rather than AdminLTE, but I will add my findings here anyway.

---

The problem is that queries with certain status types are counted twice when calculating the stats. At least when reading from the database, e.g. after pihole restartdns. Here is where the stats are calculated:
https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/api/api.c#L521-L576
The important variables to keep in mind are totalqueries, counters->blocked and upstream->count.

When queries are read form the database, the associated upstream server needs to be converted into an ID. This is done here:
https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/database/query-table.c#L390-L406
While doing so, the call of findUpstreamID() will increment upstream->count for every query with an associated upstream server, i.e. all queries with these status types (the comment in the code is not correct anymore):

QUERY_FORWARDED
QUERY_GRAVITY_CNAME
QUERY_REGEX_CNAME
QUERY_BLACKLIST_CNAME
QUERY_EXTERNAL_BLOCKED_IP
QUERY_EXTERNAL_BLOCKED_NULL
QUERY_EXTERNAL_BLOCKED_NXRA

However, further down the line, every one of the blocked queries is also incrementing the counters->blocked variable:
https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/database/query-table.c#L498-L544

This leads to the problem:
The blocked queries are counted twice in upstream->count and counters->blocked, but only go once into totalqueries. Only queries with actual status QUERY_FORWARDED are incrementing counters->forwarded. Due to this, the calculated percentages of the upstream servers are too high and everything adds up to more than 100 %.

Solution 1: Make the problematic query types increment counters->forwarded as well to balance everything out:

                [...]
        // Increment status counters
        switch(status)
        {
            case QUERY_UNKNOWN: // Unknown
                counters->unknown++;
                break;

                        // These types were originally forwarded and then blocked later.
                        // They are already counted once as part of the respective upstream server
                        // and thus need to be counted as forwarded too to make the statistics work.
                        // https://github.com/pi-hole/AdminLTE/issues/1612
            case QUERY_EXTERNAL_BLOCKED_IP: // Blocked by external provider
            case QUERY_EXTERNAL_BLOCKED_NULL: // Blocked by external provider
            case QUERY_EXTERNAL_BLOCKED_NXRA: // Blocked by external provider
            case QUERY_GRAVITY_CNAME: // Blocked by gravity (inside CNAME path)
            case QUERY_REGEX_CNAME: // Blocked by regex blacklist (inside CNAME path)
            case QUERY_BLACKLIST_CNAME: // Blocked by exact blacklist (inside CNAME path)

                                 counters->forwarded++;

            case QUERY_GRAVITY: // Blocked by gravity
            case QUERY_REGEX: // Blocked by regex blacklist
            case QUERY_BLACKLIST: // Blocked by exact blacklist
                counters->blocked++;
                query->flags.blocked = true;
                // Get domain pointer
                domainsData* domain = getDomain(domainID, true);
                domain->blockedcount++;
                change_clientcount(client, 0, 1, -1, 0);
                // Update overTime data structure
                overTime[timeidx].blocked++;
                break;


            case QUERY_FORWARDED: // Forwarded
                counters->forwarded++;
                // Update overTime data structure
                overTime[timeidx].forwarded++;
                break;
                [...]

Solution 2: Exempt these blocked queries from being counted to the respective upstream server in the first place:

            [...]
        const char *upstream = NULL;
        int upstreamID = -1; // Default if not forwarded
        // Determine upstreamID only when status == 2 (forwarded) as the
        // field need not to be filled for other query status types
        if(sqlite3_column_bytes(stmt, 6) > 0 &&
           (upstream = (const char *)sqlite3_column_text(stmt, 6)) != NULL)
        {
            // Get IP address and port of upstream destination
            char serv_addr[INET6_ADDRSTRLEN] = { 0 };
            unsigned int serv_port = 53;
            // We limit the number of bytes written into the serv_addr buffer
            // to prevent buffer overflows. If there is no port available in
            // the database, we skip extracting them and use the default port
            sscanf(upstream, "%"xstr(INET6_ADDRSTRLEN)"[^#]#%u", serv_addr, &serv_port);
            serv_addr[INET6_ADDRSTRLEN-1] = '\0';

                        // Queries with these status types were really blocked and should not
                        // be counted as being forwarded to the respective upstream server
                        // https://github.com/pi-hole/AdminLTE/issues/1612
            if(status == QUERY_EXTERNAL_BLOCKED_IP ||
               status == QUERY_EXTERNAL_BLOCKED_NULL ||
               status == QUERY_EXTERNAL_BLOCKED_NXRA ||
               status == QUERY_GRAVITY_CNAME ||
               status == QUERY_REGEX_CNAME ||
               status == QUERY_BLACKLIST_CNAME)
            {
                    upstreamID = findUpstreamID(serv_addr, (in_port_t)serv_port, false);
            }
            else
            {
                upstreamID = findUpstreamID(serv_addr, (in_port_t)serv_port, true);
            }
        }
            [...]

---

I don't know if this is also true if memory is generated live or if that is really only the case when read from the database. I didn't yet get a good understanding of the Dnsmasq interface.

[Daxtorim]

Okay, I dug a little deeper and I think I understand the Dnsmasq interface a little better. This code is also affected.

There are two places were the counters a corrected after the fact:

1. After a query was determined to be blocked after CNAME inspection:
   https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/dnsmasq_interface.c#L1415-L1439
2. After a query was determined to be externally blocked:
   https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/dnsmasq_interface.c#L1251-L1287

Notice how 2. is just calling 1. at the end. Because of that, counters->forwarded is actually decremented twice when a query was blocked externally. On the other hand, upstream->count is not corrected at all when a query was blocked after CNAME inspection. The correction of the counters can be moved from 2. to 1. and it should give a more consistent result.

static void query_blocked(queriesData* query, domainsData* domain, clientsData* client, const unsigned char new_status)
{
    // Get response time
    struct timeval response;
    gettimeofday(&response, 0);
    save_reply_type(blocking_flags, NULL, query, response);

    // Adjust counters if we recorded a non-blocking status
    if(query->status == QUERY_UNKNOWN)
    {
        counters->unknown--;
    }
    else if(query->status == QUERY_FORWARDED)
    {
               // Correct counters if necessary ...
        counters->forwarded--;
        overTime[query->timeidx].forwarded--;

        // Get forward pointer
        upstreamsData* upstream = getUpstream(query->upstreamID, true);
        if(upstream != NULL)
            upstream->count--;
    }
    else if(query->status == QUERY_CACHE)
    {
        counters->cached--;
    }
    else
    {
        // Already a blocked query, no need to change anything
        return;
    }

    // Count as blocked query
    counters->blocked++;
    overTime[query->timeidx].blocked++;
    if(domain != NULL)
        domain->blockedcount++;
    if(client != NULL)
        change_clientcount(client, 0, 1, -1, 0);

    // Update status
    query->status = new_status;
    query->flags.blocked = true;
}

*I haven't actually tested any of the modified code yet.

---

This begs the question:
Should the counters be corrected at all?
Are a blocked query and a forwarded query mutually exclusive?

These queries were forwarded and did create a load on the DNS server before they were shot down. In case they aren't exclusive and we choose Solution 1, the correction of the counters should be removed completely. This would mean that externally blocked queries and those blocked after CNAME inspection are contributing to both counters->blocked and counters->forwarded. All parts of the pie chart will be proportionally affected, they should then still be adding up to 100%. That is what this is trying to fix after all.

Solution 2 would mean we ignore the fact that these queries were forwarded at all and just pretend they were blocked immediately. In that case the counters need to be corrected of course.

(There is also FTL/src/gc.c that is also modifying these counters. Need to take a look at that as well.)

I'll see if I can find some time to do a little testing over the weekend.

Opinions on which solution to pick? Anything obvious I missed?

[pralor-bot]

This issue has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/wrong-percent-calculation-in-pie-chart/44277/8

[yubiuser]

Thanks for that digging!

Are a blocked query and a forwarded query mutually exclusive?

These queries were forwarded and did create a load on the DNS server before they were shot down. In case they aren't exclusive and we choose Solution 1, the correction of the counters should be removed completely. This would mean that externally blocked queries and those blocked after CNAME inspection are contributing to both counters->blocked and counters->forwarded

In my opinion, they are mutually exclusive. From a users point of view, I would not care about where the query was blocked but only if it was blocked. For the counters I would use the status definition from here
https://docs.pi-hole.net/database/ftl/#supported-status-types

My understanding is that everything that is blocked there should only contribute to blocked queries and not forwarded. This is in line with the current query log, where externally and CNAME blocked queries are shown in red (aka. blocked).

[DL6ER]

Thanks for the research. I agree that they are considered mutually exclusive and maybe they don't have to. In the end, the question is what the diagrams should show. With the current concept, you know that everything in the forward destination is not blocked (= permitted). The same goes for the cache. However, if we'd start mixing things (like externally blocked queries contribute to both), this sharp distinction wouldn't be there anymore. Even when this would reflect better what happened on the wire, this may be less what a user expects.

The GC function keeps all the internal counters consistent to ensure FTL can run months (or years) without the need for a restart. Any issues with the counters here would likely show up even earlier.

The modification of the counters being inlined in the find...ID subroutines was fine when FTL was a lot simpler years ago, however, meanwhile, this should be improved to avoid such mistakes. We already removed these things from the v6.0 code, however, it won't hurt to fix things in v5.x code and push a bugfix release until v6.0 becomes ready (this will take some time).

I will move this ticket into the FTL namespace as this is where it belongs.

[DL6ER]

Proposed fix is https://github.com/pi-hole/FTL/pull/1057

[Daxtorim]

@DL6ER

The GC function keeps all the internal counters consistent

Do queries with status types QUERY_RETRIED and QUERY_RETRIED_DNSSEC increment counters->forwarded at some point that I missed? Because they certainly do not increment the counter when being read from the database:
https://github.com/pi-hole/FTL/blob/bfa0d996d8e472fcd49d6bfdd49105897fabccdf/src/database/query-table.c#L537-L541

But when those queries are removed, counters->forwarded is decremented by GC:
https://github.com/pi-hole/FTL/blob/da89cc669f57fcefc6a171a5aa72c874f3ad0e78/src/gc.c#L89-L109

[DL6ER]

Do queries with status types QUERY_RETRIED and QUERY_RETRIED_DNSSEC increment counters->forwarded at some point that I missed?

Yes, the flow is the following:

1. A query is sent upstream: _FTL_forwarded is called, it does counters->forwarded++
2. The query is retried by the requesting client: FTL_forwarding_retried is called (it does not modify the counters)
   This function marks the previous query as having been retried and doesn't do anything else (exception for DNSSEC retried, but they don't count anything, either)
3. The new query is sent upstream: _FTL_forwarded is called, it does counters->forwarded++

The database code was incorrect. I pushed a fix to the same branch.

[DL6ER]

Pi-hole FTL v5.7 has been released