Skip to content

Conversation

@constantinius
Copy link
Contributor

@constantinius constantinius requested a review from a team as a code owner November 26, 2025 11:41
@linear
Copy link

linear bot commented Nov 26, 2025

@constantinius constantinius requested a review from a team November 26, 2025 11:41
Copy link
Contributor

@sentrivana sentrivana 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
if hasattr(result, "usage") and callable(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
Contributor Author

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
Contributor Author

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

codecov bot 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 lines Patch % Lines
...sdk/integrations/pydantic_ai/spans/invoke_agent.py 60.00% 4 Missing and 4 partials ⚠️
sentry_sdk/integrations/pydantic_ai/spans/utils.py 83.33% 0 Missing and 2 partials ⚠️
..._sdk/integrations/pydantic_ai/patches/agent_run.py 66.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 lines Coverage Δ
...ry_sdk/integrations/pydantic_ai/spans/ai_client.py 76.19% <100.00%> (+0.07%) ⬆️
..._sdk/integrations/pydantic_ai/patches/agent_run.py 90.47% <66.66%> (+3.11%) ⬆️
sentry_sdk/integrations/pydantic_ai/spans/utils.py 83.33% <83.33%> (ø)
...sdk/integrations/pydantic_ai/spans/invoke_agent.py 75.36% <60.00%> (-7.00%) ⬇️

... and 1 file with indirect coverage changes

@constantinius constantinius merged commit a06c9f0 into master Nov 27, 2025
154 of 155 checks passed
@constantinius constantinius deleted the constantinius/feat/integrations/pydantic-ai-invoke-agent-usage-model branch November 27, 2025 09:51
Sign up for free to 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