-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix(core): Report well known values in gen_ai.operation.name attribute #18925
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
fix(core): Report well known values in gen_ai.operation.name attribute #18925
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.
|
4bc1034 to
ea1883c
Compare
…lues-in-gen_aioperationname-attribute
| { | ||
| name: `${operationName} ${modelName}`, | ||
| op: 'gen_ai.pipeline', | ||
| op: 'gen_ai.invoke_agent', |
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.
m: isn't this for single llm calls? in that case wouldn't chat be more suitable?
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.
because langchain also has a separate create_agent API: https://docs.langchain.com/oss/python/langchain/agents#static-model
so to me it would seem that we should use chat for individual invokations and invoke_agent if the agent is invoked
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.
Makes sense, i kept going back and forth on this one. having a separate create_agent API settles it
| } | ||
| // Return the original value for unknown operations | ||
| return operationName; | ||
| } |
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.
l: I think it would be better to use sets in this method and then just do a lookup in the condition e.g. INVOKE_AGENT_OPS that contains the operations we want to map to invoke_agent
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.
Totally reasonable
…lues-in-gen_aioperationname-attribute
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 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
dev-packages/node-integration-tests/suites/tracing/vercelai/test.ts
Outdated
Show resolved
Hide resolved
nicohrubec
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.
thanks for updating, lgtm!
AI integrations should follow OTel spec and report the gen_ai.operation.name with the values listed in sentry conventions if applies getsentry/sentry-conventions#225, this PR renames gen_ai.operation so that if one of values applies, then that value MUST be used.
Closes https://linear.app/getsentry/issue/JS-1527/report-well-known-values-in-gen-aioperationname-attribute