Make HTTP requests concurrently#232
Conversation
Closes iopipe#222 Signed-off-by: Michael Lavers <kolanos@gmail.com>
pselle
left a comment
pselle
left a comment
There was a problem hiding this comment.
This is cool! Have you run any manual tests to see the difference between the agent before and after this change re: performance?
| self.handler.stream.close() | ||
| plugin['uploads'].append(self.signed_request['jwtAccess']) | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
The del isn't needed anymore because futures wait will close things if this unloads?
There was a problem hiding this comment.
Yes. The GC will also probably call __del__ before the thread is done with the stream, so this avoids a race condition as well.
|
|
||
| self.run_hooks('post:setup') | ||
|
|
||
| def __del__(self): |
There was a problem hiding this comment.
Ah, so this (versus the comment in plugins) is where unloading will happen?
There was a problem hiding this comment.
So ThreadPoolExecutor maintains a thread pool, this ensures that the pool is shutdown gracefully, which includes waiting for any futures that might still be running. This should be rare as wait_for_futures will wait for futures after each invocation. The circumstance where __del__ would be engaged is when AWS Lambda is terminating a process.
| }, | ||
| headers={ | ||
| 'Authorization': report.report['client_id'] | ||
| 'Authorization': token, |
There was a problem hiding this comment.
This is easier to read, dig it
| packages=find_packages(exclude=('tests', 'tests.*',)), | ||
| extras_require={ | ||
| 'dev': ['flake8', 'requests'], | ||
| 'dev': ['jmespath>=0.7.1,<1.0.0', 'requests'], |
There was a problem hiding this comment.
You removed flake8 as a dev requirement? Is this why there're some flake8 changes? (there are a few diffs that are reorganization)
There was a problem hiding this comment.
It should be in tests_require, will add it back in there. The dev dependencies are meant for someone running IOpipe locally, which would not require flake8.
| stream = iopipe.plugins[0].handler.stream | ||
|
|
||
| assert hasattr(stream, 'file') | ||
| assert stream.file.closed |
There was a problem hiding this comment.
Is there a way to test that the streams are closed at the end of the invocation with this new functionality?
There was a problem hiding this comment.
Because they're now closed in another thread, it is difficult to test. Will look into it.
| @@ -1,5 +1,6 @@ | |||
| import socket | |||
| import sys | |||
| import time | |||
Signed-off-by: Michael Lavers <kolanos@gmail.com>
…letion Signed-off-by: Michael Lavers <kolanos@gmail.com>
|
@pselle Regarding performance, making HTTP requests concurrently now means that the HTTP requests will now be as fast as the slowest request made. So if the agent is only sending a report, then there won't be much (if any) of a performance gain. But if the agent is sending a report, profile and logs -- then it will perform all three concurrently and will only block until the slowest request is complete. Since we're looking at adding a kitchen sink, this will greatly speed things up. |
|
@kolanos Thanks, I understand how concurrency will benefit, I was asking if you'd done any testing with regards to "how much" |
|
@pselle I have benchmarked |
Signed-off-by: Michael Lavers <kolanos@gmail.com>
2 participants
Closes #222
Signed-off-by: Michael Lavers kolanos@gmail.com