[feature] Added optional clean insights measurement collection #360

[pandafy]

Closes #360

Changes

#Please explain changes made in your PR#

Screenshots

#For UI changes, please share a screenshot#

Checklist

• I have read the contributing guidelines.
• I have manually tested the proposed changes.
• I have written new test cases to avoid regressions. (if necessary)
• I have updated the documentation. (e.g. README.rst)
• I have added [change!] to commit title to indicate a backward incompatible change. (if required)
• I have checked the links added / modified in the documentation.

#Closes #(issue-number)#

[nemesifier]

Let's use the retry mechanism offered by the requests library that you know of ;)

[nemesifier]

Let's not mention clean insights in the code and comments except in the README.
Let's not assume people reading the code know what clean insights is, let's use a generic term "Usage Metric Collection".

See my comments below.

[nemesifier]

Let's add a paragraph introducing and linking to cleaninsights.

[nemesifier]

are you absolutely sure this code works also when the server does not reply at all?

[pandafy]

I have tested the following scenarios:

1. Trying to establish connection to the port where no application is listening - Check the command output on Hastebin
2. When the server time outs (hangs) after establishing the connection - Check the command output on Hastebin

I have added a test case for this https://github.com/openwisp/openwisp-utils/pull/365/files#diff-118b67d11be9c05a77bccfedd814f41974ca3fb2bfd21eb010350faf22d59d3bR180-R196

[pandafy]

I used this code for the mock server

from http.server import SimpleHTTPRequestHandler
from socketserver import TCPServer


class MyHandler(SimpleHTTPRequestHandler):
    def do_GET(self):
        import time
        time.sleep(40000000)


def run_server():
    server_address = ('', 8000)
    httpd = TCPServer(server_address, MyHandler)

    print(f"Server running on http://localhost:{server_address[1]}")

    try:
        httpd.serve_forever()
    except KeyboardInterrupt:
        pass
    finally:
        httpd.server_close()


if __name__ == "__main__":
    run_server()

[pandafy]

I intentionally left this reference to Clean Insights here to remind us in the future why we are using this specific structure.

[coveralls]

coverage: 98.395% (+0.2%) from 98.243%
when pulling 35d0cda on issues/360-clean-insights
into 66cce46 on master.

[nemesifier]

Can you look into why the code coverage is decreasing?

[nemesifier]

I think the metric collection logic is good to merge, but I think the retryable_request needs a bit more love so we can reuse it in other OpenWISP python modules too.

[nemesifier]

This new utility function will be useful, please allow to custoimize the arguments passed to Retry(), eg:

• backoff_factor
• backoff_jitter
• status_forcelist
• allowed_methods
• add another argument called retry_kwargs=None

Then I recommend doing something like:

if retry_kwargs is None:
    retry_kwargs = {}
retry_kwargs.update(dict(
    total=max_retries,
    backoff_factor=backoff_factor,
    backoff_jitter=backoff_jitter,
    status_forcelist=status_forcelist,
    allowed_methods=allowed_methods
))
retries = Retry(**retry_kwargs)

This will allow us to reuse this function in other modules because as you know it's a common need.
Please add this function to the README and make sure there's at least one unit test dedicated to this function only.

[nemesifier]

one more time, let's not use mutable default arguments for the reasons described here:
https://dev.to/sharmapacific/mutable-default-arguments-in-python-22ce

We have already been hit a few times by obscure bugs which originated from using mutable default arguments, let's not repeat the same mistakes.

This applies to timeout, status_forcelist, allowed_methods.

[nemesifier]

After giving a second look to the mutable argument subject, I think it's ok since we don't return the mutable objects nor manipulate these inside the function, so we are not risking to be hit by weird bugs and this usage is fine.

[pandafy]

@nemesifier Tuples in Python are immutable. That's why I used tuples and not lists. 😄