-
Notifications
You must be signed in to change notification settings - Fork 7
add on chunk events to genai spans #200
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
Conversation
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.
Important
Looks good to me! 👍
Reviewed everything up to 279e9ba in 1 minute and 28 seconds. Click for details.
- Reviewed
44
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/google_genai/__init__.py:388
- Draft comment:
Adding an event per chunk is useful; consider checking if the span is recording (e.g. using span.is_recording()) before calling add_event to avoid unnecessary overhead. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. src/lmnr/opentelemetry_lib/opentelemetry/instrumentation/google_genai/__init__.py:439
- Draft comment:
Similarly, in the async streaming response, consider guarding the add_event call with a check for span.is_recording() to be consistent. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. tests/test_google_genai.py:847
- Draft comment:
The test now tracks the number of chunks (chunk_count) and later asserts that the number of span events matches; this is a clear and effective verification of the chunk event instrumentation. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. tests/test_google_genai.py:887
- Draft comment:
The additional assertions verifying that every event has the name 'llm.content.completion.chunk' effectively ensure correct event logging. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_JjqGlIvPyOH5OqUL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed fa4a934 in 1 minute and 2 seconds. Click for details.
- Reviewed
20
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/lmnr/sdk/laminar.py:674
- Draft comment:
Replace alias 'L' with 'Laminar' in the docstring example for clarity and consistency. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_l3P6EVL8w1HXXIos
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 7945365 in 1 minute and 23 seconds. Click for details.
- Reviewed
38
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_h30LBWr84cZywref
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
try: | ||
span.add_event("llm.content.completion.chunk") | ||
except Exception: | ||
pass |
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 try/except block around span.add_event swallows all exceptions. Consider logging the exception (even at a debug level) so that unexpected failures in adding events aren’t silently ignored.
pass | |
logger.debug("Exception in span.add_event: ", exc_info=True) |
try: | ||
span.add_event("llm.content.completion.chunk") | ||
except Exception: | ||
pass |
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.
Similarly, the async version wraps span.add_event in a try/except without logging. It would be beneficial to log errors for visibility, even if instrumentation should continue.
pass | |
logger.exception('Failed to add event to span') |
Important
Add event logging for chunk processing in streaming responses and update
Laminar
method calls to use full class name.llm.content.completion.chunk
to spans in_build_from_streaming_response()
and_abuild_from_streaming_response()
in__init__.py
.Laminar
method calls inlaminar.py
to use full class name instead of aliasL
.test_google_genai_streaming()
intest_google_genai.py
to verify event logging for each chunk processed.This description was created by
for 7945365. You can customize this summary. It will automatically update as commits are pushed.