Skip to content

Commit

Permalink
Propagate request start/end lazily (#1687)
Browse files Browse the repository at this point in the history
With this commit we only propagate request start and end time to the
parent context if there actually is one. Previously we unconditionally
propagated the request context which could lead to misleading errors
when there was an open request context but no request was issued. This
can happen when a mandatory parameter is missing and the runner was
throwing exception. That exception would get masked by a `KeyError`
because no request start was present yet and thus obfuscate the root
cause.
  • Loading branch information
danielmitterdorfer committed Apr 17, 2023
1 parent 1d1e1f9 commit b39af39
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 7 deletions.
12 changes: 5 additions & 7 deletions esrally/client/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,19 @@ async def __aenter__(self):

@property
def request_start(self):
return self.ctx["request_start"]
return self.ctx.get("request_start")

@property
def request_end(self):
return self.ctx["request_end"]
return self.ctx.get("request_end")

async def __aexit__(self, exc_type, exc_val, exc_tb):
# propagate earliest request start and most recent request end to parent
request_start = self.request_start
request_end = self.request_end
self.ctx_holder.restore_context(self.token)
# don't attempt to restore these values on the top-level context as they don't exist
if self.token.old_value != contextvars.Token.MISSING:
self.ctx_holder.update_request_start(request_start)
self.ctx_holder.update_request_end(request_end)
# propagate earliest request start and most recent request end to parent
self.ctx_holder.update_request_start(self.request_start)
self.ctx_holder.update_request_end(self.request_end)
self.token = None
return False

Expand Down
13 changes: 13 additions & 0 deletions tests/client/factory_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,19 @@ def test_client_cert(self, tmp_path_factory: pytest.TempPathFactory):


class TestRequestContextManager:
@pytest.mark.asyncio
async def test_does_not_propagates_empty_toplevel_context(self):
test_client = client.RequestContextHolder()
async with test_client.new_request_context() as top_level_ctx:
# Simulate that we started a request context but did not issue a request.
# This can happen when a runner throws an exception before it issued an
# API request. The most typical case is that a mandatory parameter is
# missing.
pass

assert top_level_ctx.request_start is None
assert top_level_ctx.request_end is None

@pytest.mark.asyncio
async def test_propagates_nested_context(self):
test_client = client.RequestContextHolder()
Expand Down

0 comments on commit b39af39

Please sign in to comment.