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

Log the rule which is the cause of blocking #1460

Merged
merged 2 commits into from
Apr 24, 2024

Conversation

zc-devs
Copy link
Contributor

@zc-devs zc-devs commented Apr 23, 2024

Part of #1458

Examples:

DEBUG Trie: wildcard block rule 'crashlytics.com' matched with 'firebase-settings.crashlytics.com'
DEBUG Trie: wildcard block rule 'telemetry.gfe.nvidia.com' matched with 'telemetry.gfe.nvidia.com'
DEBUG Trie: wildcard block rule 'mobile.events.data.microsoft.com' matched with 'mobile.events.data.microsoft.com'

DEBUG stringMap: block rule 'appboy.com' matched with 'appboy.com'
DEBUG stringMap: block rule 'app.adjust.com' matched with 'app.adjust.com'

Copy link
Collaborator

@ThinkChaos ThinkChaos left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I think ideally we should bubble up the info a bit further so the logs are associated with the request, and that we can even potentially return it through extended errors, or a blocky HTTP API, which would allow the blocky CLI to let you know what the rule is without needing to check logs.
To do that I'd probably update ListCache.Match to return a map[string]string which would use the group names as keys, and the rule that caused the block as value. From there, the compiler should show you what else needs to be updated up the stack, and down the stack you'd need to bridge Match and the code you've added in the caches.

That being said I think a debug like this is already a big UX improvement and can be merged if you don't want to take on extra work! No pressure either way!
In that case just fixing the lint errors is good, hopefully my suggestions do the trick :)

cache/stringcache/string_caches.go Outdated Show resolved Hide resolved
trie/trie.go Outdated Show resolved Hide resolved
cache/stringcache/string_caches.go Show resolved Hide resolved
trie/trie.go Show resolved Hide resolved
trie/trie.go Show resolved Hide resolved
Co-authored-by: ThinkChaos <[email protected]>
@zc-devs zc-devs force-pushed the 1458-improve-block-logging branch from e3f0ebc to 8eff14f Compare April 24, 2024 15:53
@zc-devs
Copy link
Contributor Author

zc-devs commented Apr 24, 2024

ideally we should bubble up the info a bit further ...

Agree. I just was afraid of changing trie.go too much.

To do that ...

Thanks for suggestions, I'll take it.

I think a debug like this is already a big UX improvement and can be merged if you don't want to take on extra work

Let's do not close the issue, but merge this PR as is. I will try to propagate the info in separate PR a little bit later.

@ThinkChaos
Copy link
Collaborator

Let's do not close the issue, but merge this PR as is. I will try to propagate the info in separate PR a little bit later.

Sure works for me!

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.22%. Comparing base (fe84ab8) to head (8eff14f).
Report is 65 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1460      +/-   ##
==========================================
+ Coverage   93.88%   94.22%   +0.34%     
==========================================
  Files          78       78              
  Lines        6361     5007    -1354     
==========================================
- Hits         5972     4718    -1254     
+ Misses        300      198     -102     
- Partials       89       91       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ThinkChaos ThinkChaos merged commit e99c98b into 0xERR0R:main Apr 24, 2024
11 checks passed
@zc-devs zc-devs deleted the 1458-improve-block-logging branch April 24, 2024 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants