Skip to content

starlette: use wrapt for patching instead of class replacement #3530

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

SrdjanLL
Copy link
Contributor

Description

Moving starlette instrumentation to patch Starlette.__init__ method using wrapt instead of doing the class replacement to prevent potential ordering issues.

Left the behaviour of manual instrumentation methods of StarletteInstrumentor to work as it did before.

The core issue where this surfaced (#3476) has already been closed, but this was still a relevant improvement

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Unit tests

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added updated
  • Documentation has been updated

@SrdjanLL SrdjanLL requested a review from a team as a code owner May 22, 2025 11:00
Comment on lines 182 to 183
from starlette import applications
from starlette.routing import Match
Copy link
Contributor

@xrmx xrmx May 22, 2025

Choose a reason for hiding this comment

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

So if we don't have an ordering issue with other instrumentations then the value of switching to wrapt would be the removal of these top level imports and push them at a later time or move them under type checking. Do you see this achievable?

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented May 26, 2025

Would love a fix to the issue, if it is this, so that we don't need to manually do StarletteInstrumentor.instrument_app(app). There are an increasing number of applications using starlette due to the prevalence of JSON-RPC in genai agents, such as Google A2A.

Right now, you have to manually instrument the app in order to use "zero code instrumentation" (and successfully join agents in the same trace). The combination of "zero code" and also needing to patch your code doesn't make any sense. cc @aabmass

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.

3 participants