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

fix(redis): handle rowcount when BaseException is raised [backport 2.9] #9900

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jul 22, 2024

Backport 8d3f827 from #9511 to 2.9.

In case of Redis raised a BaseException like asyncio.CancelledError.

ddtrace integration tries to compute the rowcount while "result"
variable does not exists.

  File "ddtrace/contrib/redis_utils.py", line 72, in _run_redis_command_async
    result = await func(*args, **kwargs)
  File "redis/asyncio/client.py", line 605, in execute_command
    conn = self.connection or await pool.get_connection(command_name, **options)
  File "redis/asyncio/connection.py", line 1076, in get_connection
    await self.ensure_connection(connection)
  File "redis/asyncio/connection.py", line 1115, in ensure_connection
    if await connection.can_read_destructive():
  File "redis/asyncio/connection.py", line 504, in can_read_destructive
    return await self._parser.can_read_destructive()
  File "redis/_parsers/hiredis.py", line 179, in can_read_destructive
    return await self.read_from_socket()
  File "redis/_parsers/hiredis.py", line 184, in read_from_socket
    buffer = await self._stream.read(self._read_size)
  File "asyncio/streams.py", line 713, in read
    await self._wait_for_data('read')
  File "asyncio/streams.py", line 545, in _wait_for_data
    await self._waiter
CancelledError: null

  File "xxxxxxx.py", line 287, in list_pulls
    await redis.zrangebyscore(
  File "ddtrace/contrib/redis/asyncio_patch.py", line 20, in traced_async_execute_command
    return await _run_redis_command_async(span=span, func=func, args=args, kwargs=kwargs)
  File "ddtrace/contrib/redis_utils.py", line 79, in _run_redis_command_async
    rowcount = determine_row_count(redis_command=redis_command, result=result)
UnboundLocalError: cannot access local variable 'result' where it is not associated with a value

This change fixes that.

Fixes #9548

Checklist

  • Change(s) are motivated and described in the PR description
  • Testing strategy is described if automated tests are not included in the PR
  • Risks are described (performance impact, potential for breakage, maintainability)
  • Change is maintainable (easy to change, telemetry, documentation)
  • Library release note guidelines are followed or label changelog/no-changelog is set
  • Documentation is included (in-code, generated user docs, public corp docs)
  • Backport labels are set (if applicable)
  • If this PR changes the public interface, I've notified @DataDog/apm-tees.

Reviewer Checklist

  • Title is accurate
  • All changes are related to the pull request's stated goal
  • Description motivates each change
  • Avoids breaking API changes
  • Testing strategy adequately addresses listed risks
  • Change is maintainable (easy to change, telemetry, documentation)
  • Release note makes sense to a user of the library
  • Author has acknowledged and discussed the performance implications of this PR as reported in the benchmarks PR comment
  • Backport labels are set in a manner that is consistent with the release branch maintenance policy

In case of Redis raised a BaseException like asyncio.CancelledError.

ddtrace integration tries to compute the rowcount while "result"
variable does not exists.

```
  File "ddtrace/contrib/redis_utils.py", line 72, in _run_redis_command_async
    result = await func(*args, **kwargs)
  File "redis/asyncio/client.py", line 605, in execute_command
    conn = self.connection or await pool.get_connection(command_name, **options)
  File "redis/asyncio/connection.py", line 1076, in get_connection
    await self.ensure_connection(connection)
  File "redis/asyncio/connection.py", line 1115, in ensure_connection
    if await connection.can_read_destructive():
  File "redis/asyncio/connection.py", line 504, in can_read_destructive
    return await self._parser.can_read_destructive()
  File "redis/_parsers/hiredis.py", line 179, in can_read_destructive
    return await self.read_from_socket()
  File "redis/_parsers/hiredis.py", line 184, in read_from_socket
    buffer = await self._stream.read(self._read_size)
  File "asyncio/streams.py", line 713, in read
    await self._wait_for_data('read')
  File "asyncio/streams.py", line 545, in _wait_for_data
    await self._waiter
CancelledError: null

  File "xxxxxxx.py", line 287, in list_pulls
    await redis.zrangebyscore(
  File "ddtrace/contrib/redis/asyncio_patch.py", line 20, in traced_async_execute_command
    return await _run_redis_command_async(span=span, func=func, args=args, kwargs=kwargs)
  File "ddtrace/contrib/redis_utils.py", line 79, in _run_redis_command_async
    rowcount = determine_row_count(redis_command=redis_command, result=result)
UnboundLocalError: cannot access local variable 'result' where it is not associated with a value
```

This change fixes that.

Fixes #9548

## Checklist

- [X] Change(s) are motivated and described in the PR description
- [x] Testing strategy is described if automated tests are not included
in the PR
- [X] Risks are described (performance impact, potential for breakage,
maintainability)
- [ ] Change is maintainable (easy to change, telemetry, documentation)
- [ ] [Library release note
guidelines](https://ddtrace.readthedocs.io/en/stable/releasenotes.html)
are followed or label `changelog/no-changelog` is set
- [ ] Documentation is included (in-code, generated user docs, [public
corp docs](https://github.com/DataDog/documentation/))
- [ ] Backport labels are set (if
[applicable](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting))
- [ ] If this PR changes the public interface, I've notified
`@DataDog/apm-tees`.

## Reviewer Checklist

- [ ] Title is accurate
- [ ] All changes are related to the pull request's stated goal
- [ ] Description motivates each change
- [ ] Avoids breaking
[API](https://ddtrace.readthedocs.io/en/stable/versioning.html#interfaces)
changes
- [ ] Testing strategy adequately addresses listed risks
- [ ] Change is maintainable (easy to change, telemetry, documentation)
- [ ] Release note makes sense to a user of the library
- [ ] Author has acknowledged and discussed the performance implications
of this PR as reported in the benchmarks PR comment
- [ ] Backport labels are set in a manner that is consistent with the
[release branch maintenance
policy](https://ddtrace.readthedocs.io/en/latest/contributing.html#backporting)

Co-authored-by: Emmett Butler <[email protected]>
(cherry picked from commit 8d3f827)
@erikayasuda erikayasuda enabled auto-merge (squash) July 22, 2024 21:28
@dnguy078
Copy link
Contributor

thank you @erikayasuda

@erikayasuda erikayasuda added the changelog/no-changelog A changelog entry is not required for this PR. label Jul 22, 2024
@datadog-dd-trace-py-rkomorn
Copy link

Datadog Report

Branch report: backport-9511-to-2.9
Commit report: a9bfd27
Test service: dd-trace-py

✅ 0 Failed, 847 Passed, 46 Skipped, 14m 47.24s Total duration (41.61s time saved)

@pr-commenter
Copy link

pr-commenter bot commented Jul 22, 2024

Benchmarks

Benchmark execution time: 2024-07-22 22:31:47

Comparing candidate commit a9bfd27 in PR branch backport-9511-to-2.9 with baseline commit 94fe5ff in branch 2.9.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 175 metrics, 9 unstable metrics.

@erikayasuda
Copy link
Contributor

Closing and re-opening to re-trigger the system tests

auto-merge was automatically disabled July 23, 2024 13:18

Pull request was closed

@erikayasuda erikayasuda reopened this Jul 23, 2024
@erikayasuda erikayasuda merged commit 809dea7 into 2.9 Jul 23, 2024
59 of 60 checks passed
@erikayasuda erikayasuda deleted the backport-9511-to-2.9 branch July 23, 2024 15:10
@dnguy078
Copy link
Contributor

Hi @erikayasuda @emmettbutler , would it be possible to cut a new release 2.9.4 that includes this change? Thank you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog A changelog entry is not required for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants