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

Add network timeout config to signer/upload requests #267

Merged
merged 1 commit into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions iopipe/contrib/logger/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ def post_invoke(self, event, context):
self.handler.flush()
if self.handler.stream.tell():
self.signed_request = self.iopipe.submit_future(
get_signed_request, self.iopipe.config["token"], self.context, ".log"
get_signed_request, self.iopipe.config, self.context, ".log"
)
if self.redirect_stdout is True:
sys.stdout = sys.__stdout__
Expand All @@ -96,7 +96,10 @@ def pre_report(self, report):
and "signedRequest" in self.signed_request
):
self.iopipe.submit_future(
upload_log_data, self.signed_request["signedRequest"], stream
upload_log_data,
self.signed_request["signedRequest"],
stream,
self.iopipe.config,
)
if "jwtAccess" in self.signed_request:
plugin = next((p for p in report.plugins if p["name"] == self.name))
Expand Down
11 changes: 8 additions & 3 deletions iopipe/contrib/logger/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,26 @@
logger = logging.getLogger(__name__)


def upload_log_data(url, stream_or_file):
def upload_log_data(url, stream_or_file, config):
"""
Uploads log data to IOpipe.

:param url: The signed URL
:param stream_or_file: The log data stream or file
:param config: The IOpipe config
"""
try:
logger.debug("Uploading log data to IOpipe")
if isinstance(stream_or_file, StringIO):
stream_or_file.seek(0)
response = requests.put(url, data=stream_or_file, timeout=None)
response = requests.put(
url, data=stream_or_file, timeout=config["network_timeout"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, do we want to use the same network timeout as collector? Or have it be a separate config item? The default value for this is 5000ms and I think we can be much more aggressive with the signer API (100ms)

Copy link
Contributor

Choose a reason for hiding this comment

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

(honestly, could be that we should make that 5000 much smaller)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the service characteristics of the collector and signer are different enough to warrant a significantly different timeout. If both services are healthy these timeouts should never be hit. The timeout acts as a n upper bound, or worst case, so would be conservative here. I'm not even sure 5s is going to be enough for S3 uploads.

)
else:
with open(stream_or_file, "rb") as data:
response = requests.put(url, data=data, timeout=None)
response = requests.put(
url, data=data, timeout=config["network_timeout"]
)
response.raise_for_status()
except Exception as e:
logger.debug("Error while uploading log data: %s", e)
Expand Down
6 changes: 2 additions & 4 deletions iopipe/contrib/profiler/plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,7 @@ def pre_invoke(self, event, context):

if self.enabled:
self.signed_request = self.iopipe.submit_future(
get_signed_request,
self.iopipe.config["token"],
self.context,
".cprofile",
get_signed_request, self.iopipe.config, self.context, ".cprofile"
)
self.profile = profile.Profile()
self.profile.enable()
Expand All @@ -82,6 +79,7 @@ def pre_report(self, report):
upload_profiler_report,
self.signed_request["signedRequest"],
stats_file.name,
self.iopipe.config,
)
self.stats_file = stats_file.name
if "jwtAccess" in self.signed_request:
Expand Down
7 changes: 4 additions & 3 deletions iopipe/contrib/profiler/request.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,18 @@
logger = logging.getLogger(__name__)


def upload_profiler_report(url, filename):
def upload_profiler_report(url, filename, config):
"""
Uploads a profiler report to IOpipe

:param url: The signed URL
:param filename: The profiler report file.
:param filename: The profiler report file
:param config: The IOpipe config
"""
try:
logger.debug("Uploading profiler report to IOpipe")
with open(filename, "rb") as data:
response = requests.put(url, data=data)
response = requests.put(url, data=data, timeout=config["network_timeout"])
response.raise_for_status()
except Exception as e:
logger.debug("Error while uploading profiler report: %s", e)
Expand Down
9 changes: 5 additions & 4 deletions iopipe/signer.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ def get_signer_hostname():
return "signer.{region}.iopipe.com".format(region=region)


def get_signed_request(token, context, extension):
def get_signed_request(config, context, extension):
"""
Returns a signed request URL from IOpipe

:param token: The IOpipe token.
:param config: The IOpipe config
:param context: The AWS context to request a signed URL
:param extension: The extension of the file to sign.
:param extension: The extension of the file to sign
:returns: A signed request URL
:rtype: str
"""
Expand All @@ -46,7 +46,8 @@ def get_signed_request(token, context, extension):
"timestamp": int(time.time() * 1000),
"extension": extension,
},
headers={"Authorization": token},
headers={"Authorization": config["token"]},
timeout=config["network_timeout"],
)
response.raise_for_status()
except Exception as e:
Expand Down
6 changes: 3 additions & 3 deletions tests/contrib/logger/test_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,10 @@ def test__logger_plugin(

handler({}, mock_context)

mock_get_signed_request.assert_called_once_with(
iopipe.config["token"], mock.ANY, ".log"
mock_get_signed_request.assert_called_once_with(iopipe.config, mock.ANY, ".log")
mock_upload_log_data.assert_called_once_with(
"https://mock_signed_url", mock.ANY, iopipe.config
)
mock_upload_log_data.assert_called_once_with("https://mock_signed_url", mock.ANY)

plugin = next((p for p in iopipe.report.plugins if p["name"] == "logger"))
assert plugin["uploads"][0] == "foobar"
Expand Down
2 changes: 1 addition & 1 deletion tests/contrib/logger/test_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ def test__upload_log_data__deletes_file(mock_requests):
temp_file = tempfile.NamedTemporaryFile(delete=False)
temp_file.close()

upload_log_data("", temp_file.name)
upload_log_data("", temp_file.name, {"network_timeout": 5})

assert not os.path.exists(temp_file.name)
4 changes: 2 additions & 2 deletions tests/contrib/profiler/test_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ def test__profiler_plugin(
handler({}, mock_context)

mock_get_signed_request.assert_called_once_with(
iopipe.config["token"], mock.ANY, ".cprofile"
iopipe.config, mock.ANY, ".cprofile"
)
mock_upload_profiler_report.assert_called_once_with(
"https://mock_signed_url", mock.ANY
"https://mock_signed_url", mock.ANY, iopipe.config
)

plugin = next((p for p in iopipe.report.plugins if p["name"] == "profiler"))
Expand Down
2 changes: 1 addition & 1 deletion tests/contrib/profiler/test_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,6 @@ def test__upload_profiler_report__deletes_file(mock_requests):
temp_file = tempfile.NamedTemporaryFile(delete=False)
temp_file.close()

upload_profiler_report("", temp_file.name)
upload_profiler_report("", temp_file.name, {"network_timeout": 5})

assert not os.path.exists(temp_file.name)