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

Feature: Batch audit requests #977

Merged

Conversation

lightswitch05
Copy link
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.

@DL6ER
Copy link
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
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>
Copy link
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
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

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