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(sanic): support version 21.x (#2240) #2379

Merged
merged 28 commits into from
Jun 17, 2021
Merged

Conversation

nizox
Copy link
Contributor

@nizox nizox commented Apr 29, 2021

Description

The latest major sanic release (21.x) introduced support for streaming responses and our instrumentation broke due to a change of method's signature (reported by #2240). This PR updates our instrumentation to support both the latest version and older versions.

Sanic 20.12 and older versions have Sanic.handle_request(request, write_callback, stream_callback) while Sanic 21 and newer versions have Sanic.handle_request(request). In both cases, a request starts when this method is called and terminates when the method returns. This PR changes the request span finish when the handle_request method returns instead of when either write_callback or stream_callback is called.

A Pin is also attached to the request.ctx object to pass the tracer to newly instrumented methods:

  • Request.respond replacing write_callback & stream_callback in Sanic 21.
  • Sanic._run_request_middleware because request.match_info is not filled anymore before Sanic.handle_request is called in Sanic 21.

Checklist

  • Added to the correct milestone.
  • Tests provided or description of manual testing performed is included in the code or PR.
  • Library documentation is updated.
  • Corp site documentation is updated (link to the PR).

@nizox nizox requested a review from a team as a code owner April 29, 2021 18:45
Copy link
Contributor

@Yun-Kim Yun-Kim left a comment

Choose a reason for hiding this comment

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

From the changes it's not immediately clear to me what the breaking change in sanic was, as well as our fix itself. Could you add some more details on the problem/solution to the PR description? 😄

ddtrace/contrib/sanic/patch.py Show resolved Hide resolved
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👍 great stuff! Just a few suggestions about the tests 🙂

tests/contrib/sanic/test_sanic_server.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Kyle-Verhoog
Kyle-Verhoog previously approved these changes May 5, 2021
Copy link
Member

@Kyle-Verhoog Kyle-Verhoog left a comment

Choose a reason for hiding this comment

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

👍 👍 great stuff @nizox!

tests/contrib/sanic/test_sanic_server.py Show resolved Hide resolved
jd
jd previously requested changes May 6, 2021
releasenotes/notes/fix-sanic-21-2d24b817e010ed84.yaml Outdated Show resolved Hide resolved
Kyle-Verhoog
Kyle-Verhoog previously approved these changes May 6, 2021
Yun-Kim
Yun-Kim previously approved these changes May 7, 2021
Copy link
Contributor

@Yun-Kim Yun-Kim left a comment

Choose a reason for hiding this comment

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

LGTM 😎

@Yun-Kim Yun-Kim dismissed jd’s stale review May 7, 2021 16:01

Specified version in release note

@nizox
Copy link
Contributor Author

nizox commented May 10, 2021

Tests are broken because pytest-sanic is currently not compatible with sanic 21. The issue is tracked on yunstanford/pytest-sanic#50.

@nizox
Copy link
Contributor Author

nizox commented May 14, 2021

A fix for pytest-sanic was pushed to yunstanford/pytest-sanic#52.

@nizox nizox dismissed stale reviews from Yun-Kim and Kyle-Verhoog via c3792bc May 20, 2021 10:26
@nizox nizox changed the title fix(sanic): support latest version (#2240) fix(sanic): support version 21.x (#2240) May 20, 2021
brettlangdon
brettlangdon previously approved these changes May 20, 2021
Yun-Kim
Yun-Kim previously approved these changes May 20, 2021
Kyle-Verhoog
Kyle-Verhoog previously approved these changes May 20, 2021
@Kyle-Verhoog Kyle-Verhoog added this to the 0.50.0 milestone May 20, 2021
@nizox nizox dismissed stale reviews from Kyle-Verhoog, Yun-Kim, and brettlangdon via 0b0d273 May 20, 2021 18:09
Kyle-Verhoog
Kyle-Verhoog previously approved these changes May 24, 2021
@mergify mergify bot merged commit 7a60617 into DataDog:master Jun 17, 2021
@nizox nizox deleted the sanic-21 branch July 22, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sanic: IndexError tuple index out of range with latest version of sanic
6 participants