-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(tracing): add system prompt, model to google genai #18424
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: develop
Are you sure you want to change the base?
fix(tracing): add system prompt, model to google genai #18424
Conversation
c076192 to
ff94aff
Compare
size-limit report 📦
|
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
ff94aff to
75f40f5
Compare
75f40f5 to
63be4b7
Compare
63be4b7 to
0a5ed6e
Compare
| span.setAttributes({ [GEN_AI_REQUEST_MESSAGES_ATTRIBUTE]: truncatedMessage }); | ||
| if (Array.isArray(params.message)) { | ||
| messages.push(...params.message); | ||
| } else { |
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: Part arrays incorrectly spread as individual messages
When params.message is Part[] (array of parts for a single user message), the code spreads each Part object as a separate item in the messages array. However, Part objects like { text: "hello" } don't have content: string or parts: Array properties required by the truncation logic in truncateGenAiMessages. When truncation is needed, these Part objects fail both isContentMessage and isPartsMessage checks, causing truncateSingleMessage to return an empty array and silently drop the data. The Part[] should instead be wrapped as { role: 'user', parts: params.message } to match the Google GenAI Content format. The same issue affects contents when it's PartUnion[].
Additional Locations (1)
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.
Changes look good to me. Maybe we could also update the main scenario in our node integration tests to verify that this works as expected?
Move the message reformatting into a separate util for google-genai, and add unit test coverage for that file. Add an integration test scenario to ensure that the system message will be included if provided in the config params. Related to getsentry/testing-ai-sdk-integrations#10 Fix JS-1218
0a5ed6e to
cd250d2
Compare
Move the message reformatting into a separate util for google-genai, and add unit test coverage for that file. Add an integration test scenario to ensure that the system message will be included if provided in the config params. Related to getsentry/testing-ai-sdk-integrations#10 Fix JS-1218
cd250d2 to
47e85eb
Compare
Move the message reformatting into a separate util for google-genai, and add unit test coverage for that file. Add an integration test scenario to ensure that the system message will be included if provided in the config params. Related to getsentry/testing-ai-sdk-integrations#10 Fix JS-1218
47e85eb to
e2a9837
Compare
Related to https://github.com/getsentry/testing-ai-sdk-integrations/pull/10
Fix JS-1218