-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): simplify truncation logic to only keep the newest message #18906
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
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.
|
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
RulaKhaled
left a comment
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.
Overall looks really good. Two things we should do:
- Take care of embeddings input
- Remove the media filtering logic entirely (lmk if that's something you want to do as a follow up, we can create a ticket and tackle it after)
| let bytesUsed = 0; | ||
| let startIndex = stripped.length; // Index where the kept suffix starts | ||
| // Strip inline media from the single message | ||
| const stripped = stripInlineMediaFromMessages([lastMessage]); |
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.
We should entirely remove inline media logic
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.
will do in a follow-up if need be
| 'gen_ai.operation.name': 'embeddings', | ||
| 'sentry.op': 'gen_ai.embeddings', | ||
| 'gen_ai.system': 'openai', | ||
| 'gen_ai.request.messages': '["This is a small input that fits within the limit"]', |
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.
so yah we don't want this format, we should map this to https://getsentry.github.io/sentry-conventions/generated/attributes/gen_ai.html#gen_aiembeddingsinput instead of gen_ai.request.messages, let's take care of this here so that request messages type is consistent with the type listed in https://getsentry.github.io/sentry-conventions/generated/attributes/gen_ai.html#gen_aiinputmessages
for truncate message logic for embeddings, according to TEL experience team, we don't need to worry about this just yet.
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 be good now. When setting the request attributes I now set embeddings inputs separately and it is excluded from the truncation
RulaKhaled
left a comment
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.
lgtm, thanks Nico
Changes
gen_ai.embeddings.inputand do not truncate that.Test Updates
Closes #18916