Skip to content

Conversation

@constantinius
Copy link
Contributor

@constantiniusconstantinius requested a review from a team as a code ownerNovember 26, 2025 11:41
@linear
Copy link

linearbot commented Nov 26, 2025

@constantiniusconstantinius requested a review from a teamNovember 26, 2025 11:41
Copy link
Contributor

@sentrivanasentrivana left a comment

Choose a reason for hiding this comment

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

Thanks, looks good to me, see one comment.

Side question: would you say this is more of a feature or a bugfix? We have a fancy new changelog generation system and I want to tag this properly 😄

# Set token usage data if available
ifhasattr(result, "usage") andcallable(result.usage):
try:
usage=result.usage()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can calling usage() have any (side-)effects that we wouldn't want to trigger?

I'm thinking something in the vein of extra DB access, API request, etc. We had a similar case in Django where we were trying to get the connection params to the DB and in some users' case that actually resulted in some expensive operations being performed.

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

No, this literally has the comment "should be a property". It is just an accessor for a nested field.

@constantinius
Copy link
ContributorAuthor

Side question: would you say this is more of a feature or a bugfix? We have a fancy new changelog generation system and I want to tag this properly 😄

It adds new fields, but those were kind of expected. So we can consider this a bugfix

@codecov
Copy link

codecovbot commented Nov 26, 2025

Codecov Report

❌ Patch coverage is 69.44444% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.98%. Comparing base (7b7ea33) to head (a22fea6).
⚠️ Report is 7 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing linesPatch %Lines
...sdk/integrations/pydantic_ai/spans/invoke_agent.py60.00%4 Missing and 4 partials ⚠️
sentry_sdk/integrations/pydantic_ai/spans/utils.py83.33%0 Missing and 2 partials ⚠️
..._sdk/integrations/pydantic_ai/patches/agent_run.py66.66%0 Missing and 1 partial ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #5153 +/- ## ======================================= Coverage 83.97% 83.98% ======================================= Files 180 181 +1 Lines 18222 18241 +19 Branches 3235 3239 +4 ======================================= + Hits 15302 15319 +17  Misses 1929 1929 - Partials 991 993 +2 
Files with missing linesCoverage Δ
...ry_sdk/integrations/pydantic_ai/spans/ai_client.py76.19% <100.00%> (+0.07%)⬆️
..._sdk/integrations/pydantic_ai/patches/agent_run.py90.47% <66.66%> (+3.11%)⬆️
sentry_sdk/integrations/pydantic_ai/spans/utils.py83.33% <83.33%> (ø)
...sdk/integrations/pydantic_ai/spans/invoke_agent.py75.36% <60.00%> (-7.00%)⬇️

... and 1 file with indirect coverage changes

@constantiniusconstantinius merged commit a06c9f0 into masterNov 27, 2025
154 of 155 checks passed
@constantiniusconstantinius deleted the constantinius/feat/integrations/pydantic-ai-invoke-agent-usage-model branch November 27, 2025 09:51
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@constantinius@sentrivana