-
Notifications
You must be signed in to change notification settings - Fork 562
feat(ai): Add python-genai
integration
#4891
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
base: master
Are you sure you want to change the base?
Conversation
e265d62
to
48a0c71
Compare
("frequency_penalty", SPANDATA.GEN_AI_REQUEST_FREQUENCY_PENALTY), | ||
("seed", SPANDATA.GEN_AI_REQUEST_SEED), | ||
]: | ||
if hasattr(config, param): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential bug: The function set_span_data_for_request
calls hasattr(config, ...)
without checking if config
is None
, causing an AttributeError
if the optional config parameter is omitted.
-
Description: In
set_span_data_for_request
, theconfig
object is retrieved fromkwargs
usingkwargs.get("config")
. If a user does not provide aconfig
object in their API call, which is a valid use case,config
will beNone
. The subsequent loop attempts to access attributes on thisNone
object withhasattr(config, param)
, which raises anAttributeError
. Because this integration code runs within a user's application, this unhandled exception will crash the application. -
Suggested fix: Before the loop that iterates over configuration parameters in
set_span_data_for_request
, add a check to ensureconfig
is notNone
. Ifconfig
isNone
, the entire loop that sets span data from the config should be skipped.
severity: 0.85, confidence: 0.98
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasattr(None, 'something')
won't raise an AttributeError
, it'll simply be False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TY for the PR Vjeran!
I'll do a full review shortly, just a couple of things I noticed on the fly:
- The new test suite is not running in CI. Check https://github.com/getsentry/sentry-python/blob/master/scripts/populate_tox/README.md#add-a-new-test-suite (the
Add a new test suite
section specifically) for what's missing in order to make this happen - Have a look at what mypy's complaining about in the Lint Sources step in CI
Lmk if anything is unclear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋🏻 Back with a big review. Overall looks nice and clean! Don't be fooled by the number of comments, it's just that it's a big PR, and I'm pointing out some stuff multiple times.
sentry_sdk/integrations/__init__.py
Outdated
"sentry_sdk.integrations.flask.FlaskIntegration", | ||
"sentry_sdk.integrations.gql.GQLIntegration", | ||
"sentry_sdk.integrations.graphene.GrapheneIntegration", | ||
"sentry_sdk.integrations.google_genai.GoogleGenAIIntegration", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the integration here will make it auto-enabling (the user wouldn't have to add GoogleGenAIIntegration
manually to their init
) -- however for new integrations, I strongly prefer to make them opt-in first, in order to iron out issues, and only make them auto-enabling after some time.
setup.py
Outdated
"statsig": ["statsig>=0.55.3"], | ||
"tornado": ["tornado>=6"], | ||
"unleash": ["UnleashClient>=6.0.1"], | ||
"google-genai": ["google-genai"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"google-genai": ["google-genai"], | |
"google-genai": ["google-genai>=1.0.0"], |
We should also have the lower bound here, to make sure it's not possible to install the sdk with an unsupported version of google-genai.
from sentry_sdk.tracing import Span | ||
|
||
|
||
def prepare_generate_content_args(args, kwargs): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we move this function to google_genai/utils.py
since it's also used by the non-streaming wrappers?
("frequency_penalty", SPANDATA.GEN_AI_REQUEST_FREQUENCY_PENALTY), | ||
("seed", SPANDATA.GEN_AI_REQUEST_SEED), | ||
]: | ||
if hasattr(config, param): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasattr(None, 'something')
won't raise an AttributeError
, it'll simply be False
if kwargs.get("stream", False): | ||
span.set_data(SPANDATA.GEN_AI_RESPONSE_STREAMING, True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also want to add the attribute for non-streaming responses?
if kwargs.get("stream", False): | |
span.set_data(SPANDATA.GEN_AI_RESPONSE_STREAMING, True) | |
span.set_data(SPANDATA.GEN_AI_RESPONSE_STREAMING, kwargs.get("stream") or False) |
|
||
return result | ||
except Exception as exc: | ||
capture_exception(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably set span status to error here, also in the sync version below.
try: | ||
from google import genai | ||
from google.genai import types as genai_types | ||
from google.genai.models import Models | ||
except ImportError: | ||
# If google.genai is not installed, skip the tests | ||
pytest.skip("google-genai not installed", allow_module_level=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usual way of checking whether the correct package is installed in a test suite is to put an importorskip
in __init__.py
, see e.g. https://github.com/getsentry/sentry-python/blob/master/tests/integrations/celery/__init__.py, and then you don't need to guard the imports in the test file.
yield models_instance | ||
|
||
|
||
def setup_mock_generate_content(mock_instance, return_value=None, side_effect=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally the tests would test that the integration does the wrapping correctly. This util seems to apply the integration wrapper on its own?
|
||
|
||
@pytest.fixture | ||
def mock_models_instance(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see we're mocking/populating a lot of internal google genai stuff in the tests. Generally, in tests we try to be as end-to-end as possible, with as little mocking as possible -- mocking takes away our ability to detect regressions/incompatibilities if something changes upstream.
I know these AI integrations are a pain to test in general, because we can't make actual requests to the API. However, even in such cases we should generally aim for mocking as early in the chain as possible (e.g., just mocking the actual json response from the API, but leaving the actual machinery of the library that uses that response intact).
Do you think it's possible to set the tests up to mock less of the internals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let me try to do that, I haven't even thought of it, but definitely makes sense to do it
f0f32d2
to
9e03a64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Token Count Inflation in Streaming Responses
The accumulate_streaming_response
function incorrectly sums token counts from streaming chunks. Since these counts (especially input and cached tokens) are often cumulative or constant across chunks, summing them leads to inflated and inaccurate usage metrics.
sentry_sdk/integrations/google_genai/streaming.py#L507-L526
sentry_sdk/integrations/google_genai/streaming.py#L69-L76
sentry-python/sentry_sdk/integrations/google_genai/streaming.py
Lines 69 to 76 in 1c5aa1c
# Accumulate token usage | |
extracted_usage_data = extract_usage_data(chunk) | |
total_input_tokens += extracted_usage_data["input_tokens"] | |
total_output_tokens += extracted_usage_data["output_tokens"] | |
total_cached_tokens += extracted_usage_data["input_tokens_cached"] | |
total_reasoning_tokens += extracted_usage_data["output_tokens_reasoning"] | |
total_tokens += extracted_usage_data["total_tokens"] |
texts = [] | ||
for candidate in response.candidates: | ||
if not hasattr(candidate, "content") or not hasattr(candidate.content, "parts"): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Nested Attribute Access Fails
The code can raise an AttributeError
when accessing nested attributes, such as candidate.content.parts
. This occurs if an intermediate attribute (like content
) is missing, or if the final attribute (parts
) is None
when an iterable is expected. This pattern is visible in _extract_response_text
.
Additional Locations (1)
@sentrivana i have made the changes regarding all the feedback, and did some more refactoring, to make the code more consistent and a bit easier to read. Also, now the pipeline is finally green 🥳 |
Adds support for
python-genai
integrations. It supports both sync and async clients, and both regular and streaming modes for interacting with models and building agents.Closes PY-1733: Add agent monitoring support for
google-genai