5 ms·

Seems to be more 'filtering access logs by a blacklist' than actually detecting bots. I run a VPN through Hetzner, so requests from my IP are not a bot (I hope

by orf 10y ago

Seems to be more 'filtering access logs by a blacklist' than actually detecting bots. I run a VPN through Hetzner, so requests from my IP are not a bot (I hope!). Really you want to look at the paths (filtering out all the /w00tw00t requests) and the user agents above all, which the author touches on. However a whitelist approach is better than a blacklist IMO. Also in the `in_block` you really want to hoist the `IPAddress(ip)` call out of the `any()` loop!

js2 10y ago
It also doesn't short-circuit as written since it uses a list comprehension. Switching to a generator solves that: def in_block(ip, block): ip_addr = IPAddress(ip) return any(ip_addr in IPNetwork(cidr) for cidr in block) Or just be explicit: def in_block(ip, block): ip_addr = IPAddress(ip) for cidr in block: if ip_addr in IPNetwork(cidr): return True return False
marklit 10y ago
Thanks for the heads up, I've updated the gist.
- guitarbill 10y ago
  Loved the write-up. Two more things that might make your Python code less memory intensive. Instead of `open(filename, 'r+b').read().split('\n')` you can use `for line in open(filename):`, which avoids loading large files into memory. (small gotcha: `line` will contain the newline character(s) which can be stripped via e.g. `rstrip`) You can also drop the square brackets from calls to methods that take an iterable e.g. `any`/`all`, `set`, and `join`. So `join([...])` becomes simply `join(...)`. Python will use a generator expressions instead of constructing and passing a new list list. To quote PEP 289: "generator expressions [are] a high performance, memory efficient generalization of list comprehensions" These can really make a difference with big files/lists, but are a good habit in any case. I hope it helps in the future!

by orf 10y ago

js2 10y ago

It also doesn't short-circuit as written since it uses a list comprehension. Switching to a generator solves that: def in_block(ip, block): ip_addr = IPAddress(ip) return any(ip_addr in IPNetwork(cidr) for cidr in block) Or just be explicit: def in_block(ip, block): ip_addr = IPAddress(ip) for cidr in block: if ip_addr in IPNetwork(cidr): return True return False

marklit 10y ago

Thanks for the heads up, I've updated the gist.

guitarbill 10y ago
Loved the write-up. Two more things that might make your Python code less memory intensive. Instead of `open(filename, 'r+b').read().split('\n')` you can use `for line in open(filename):`, which avoids loading large files into memory. (small gotcha: `line` will contain the newline character(s) which can be stripped via e.g. `rstrip`) You can also drop the square brackets from calls to methods that take an iterable e.g. `any`/`all`, `set`, and `join`. So `join([...])` becomes simply `join(...)`. Python will use a generator expressions instead of constructing and passing a new list list. To quote PEP 289: "generator expressions [are] a high performance, memory efficient generalization of list comprehensions" These can really make a difference with big files/lists, but are a good habit in any case. I hope it helps in the future!