Skip to content

Feature: Batch audit requests#977

Merged
AzureMarker merged 1 commit into
pi-hole:develfrom
lightswitch05:feature/batch-audit-requests
Jul 7, 2019
Merged

Feature: Batch audit requests#977
AzureMarker merged 1 commit into
pi-hole:develfrom
lightswitch05:feature/batch-audit-requests

Conversation

@lightswitch05
Copy link
Copy Markdown
Member

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

Auditing gets a bit slow on the RPi 3+ after the audit list gets large enough. Often, when I audit domains, I'll click audit quickly on many items in the list and then wait for the refresh. This change batches all the audit requests into a single request (audit actions taken within a three second grace period) - theoretically using less resources.

How does this PR accomplish the above?:

The API already supports adding multiple domains at once to a list. Since the API already supports it, all I had to do was add a timeout when auditing domains to wait and see if the user is auditing more then one domain or not. I ended up using three seconds to wait for more auditing domains - I feel like this is a decent trade-off between slowing down single-audit users vs. multi-audit users. If the user is just clicking the same domain multiple times - then it is ignore and more time is not added to the timeout.

What documentation changes (if any) are needed to support this PR?:

No documentation changes needed as far as I'm aware.

@lightswitch05 lightswitch05 force-pushed the feature/batch-audit-requests branch from bba9db3 to f407edf Compare July 5, 2019 04:07
@lightswitch05 lightswitch05 reopened this Jul 5, 2019
@DL6ER DL6ER requested review from AzureMarker and DL6ER July 5, 2019 10:48
@DL6ER
Copy link
Copy Markdown
Member

DL6ER commented Jul 5, 2019

The proposed changes look fine and the idea is good, too. However, I'm unable to test the proposed changes right now and can obviously only approve after I tested the code. I also requested a review from @Mcat12 who might be in a better position for tests.

Note that we're currently discussing whether we want to keep the auditing feature or remove it in a future release of Pi-hole as it introduced a significant complexity to FTL's domain processing and accordignly reduces maintainability. This seems to come at a high price for a rarely used feature.
We have never really documented the feature, either. If you like and/or regularly use the domain auditing functionality, it would be nice if you could describe your particular use case in a few sentences.

@lightswitch05
Copy link
Copy Markdown
Member Author

lightswitch05 commented Jul 5, 2019

Hey @DL6ER thank you for the review. The audit log is one of my main sources for the lists that I maintain: https://www.github.developerdan.com/hosts

It is particularly useful for ad/tracking domains used by apps- which are more difficult the keep an eye on then standard websites (I use uMatrix often on my laptop). I believe other list curators also use the audit feature. I go into the audit page almost daily- I quickly audit the sub domains of the known hosts- like Netflix - and then I review any leftover domains. If I’m unsure if one is an ad or tracking domain, I’ll often add it to my own blacklist for a few weeks before including it in my public list. Quickly auditing the know subdomains is what led me to this pull request. More regex filters could reduce the size of my lists greatly and reduce the usefulness of the audit feature- but I use regex filters sparingly and mostly for only random sub domains. It is my understanding that hosts lists are much quicker and have less speed impact on the overall runtime of the FTL then regex. Also, regex lists can’t be shared like hosts lists- and I think that is probably a good thing.

Personally, I would consider the loss of the audit feature a major regression and I would likely not upgrade to newer version unless I were forced to due to other bugs/issues

Signed-off-by: Daniel <daniel@developerdan.com>
@lightswitch05 lightswitch05 force-pushed the feature/batch-audit-requests branch from d74a0cb to 12e0ce2 Compare July 5, 2019 14:24
Copy link
Copy Markdown
Contributor

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works as expected.

@AzureMarker AzureMarker merged commit f18e287 into pi-hole:devel Jul 7, 2019
AzureMarker added a commit that referenced this pull request Sep 11, 2019
@AzureMarker AzureMarker added this to the v4.3.2 milestone Sep 12, 2019
@lightswitch05
Copy link
Copy Markdown
Member Author

Note that we're currently discussing whether we want to keep the auditing feature or remove it in a future release of Pi-hole as it introduced a significant complexity to FTL's domain processing and accordignly reduces maintainability.

I've been thinking about this more. I use the audit feature extensively, and I know some other blocklist maintainers do as well. However, I think it could be improved drastically. For example, I never want to see certain domains in the audit log: n0.njffgngvp.pbz.w.02.a.sophosxl.net. These are basically random and just pollute the audit. The solution would be to add wildcard auditing which I doubt you guys care about implementing and supporting.

Considering the new API you've been working on, breaking features like the audit-log out into its own standalone application makes a lot more sense. The audit feature could easily be a simple add-on web app that works along side of the pihole

@pralor-bot
Copy link
Copy Markdown

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

https://discourse.pi-hole.net/t/pi-hole-4-3-2-release-notes/23852/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants