[WIP] feat: add semantic convention support for anthropic sdk - #175
[WIP] feat: add semantic convention support for anthropic sdk#175123liuziming wants to merge 2 commits into
Conversation
|
musi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
|
This PR has been automatically marked as stale because it has not had any activity for 14 days. It will be closed if no further activity occurs within 14 days of this comment. |
ralf0131
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
Adds a new instrumentation plugin for the Anthropic Python SDK (messages.create / AsyncMessages.create, including SSE streaming) following the OTel contrib pattern. Includes LICENSE, examples, and cassette-based tests covering basic/error/stop-reason/token-usage paths. Nice, clean structure overall.
Note: I see the PR is marked [WIP], so I've focused on design/architecture feedback rather than nitpicks — the findings below should hold even if individual lines change.
Findings
- [Warning]
patch.py—AnthropicStreamWrapperandAsyncAnthropicStreamWrapperduplicate ~120 lines (_process_event,_cleanup,_ContentBlockAccumulatorusage, and all state fields) almost verbatim. This is a real maintenance hazard: when a token-usage or content-block edge case is fixed in one, the other is easy to forget. Consider extracting a shared_StreamTelemetryMixin/base that both sync and async wrappers inherit, with only the__next__/__anext__plumbing differing. - [Info] Streaming span lifecycle: finalization (
_cleanup→stop_llm/fail_llm) only runs via__next__hittingStopIteration,__exit__, or explicitclose(). If a caller partially consumes a stream and discards the iterator without using it as a context manager / callingclose(), the LLM span is never ended (leak). There's no__del__safety net. Worth at least documenting that streaming must be consumed in awithblock, or adding a best-effort__del__. - [Info]
build_output_messagesusesfinish_reason=stop_reason or "error". If a stream completes normally butstop_reasonwasn't captured by the time_cleanupruns, a successful response is labelled"error"— a false-positive error finish reason that would skew metrics. Consider distinguishing "absent" from "error". - [Info] License header in
patch.pyis the full Apache header (good), but it's inconsistent with the sibling pydantic-ai / deepagents plugins which truncate to two lines. Aligning helps if a license-header lint is ever enabled.
Suggestions
Something like:
class _StreamTelemetryMixin:
def _process_event(self, event): ... # shared
def _cleanup(self, error=None): ... # shared
class AnthropicStreamWrapper(_StreamTelemetryMixin): ...
class AsyncAnthropicStreamWrapper(_StreamTelemetryMixin): ...Cross-repo Note
No protocol/API shared with loongsuite-pilot; no cross-repo change needed.
Automated review by github-manager-bot
|
This PR has conflicts with the base branch and cannot be merged. Please rebase or merge the base branch into your branch and resolve the conflicts. This is a one-time reminder. Feel free to @mention me for a re-review after conflicts are resolved. Automated notification by github-manager-bot |
|
CLA Not Signed The Contributor License Agreement (CLA) check is currently pending on this PR ( @123liuziming please sign the CLA via the CLA assistant badge in the comment above, or visit https://cla-assistant.io/alibaba/loongsuite-python. Once signed, the Automated check by github-manager-bot |
ralf0131
left a comment
There was a problem hiding this comment.
Summary
This PR adds semantic convention support for the Anthropic SDK, aligning LoongSuite's instrumentation with upstream GenAI semantic conventions. The changes are comprehensive (30 files, 5400+ lines).
This PR cannot be approved/merged until the following issues are resolved:
- CLA not signed — Please sign the Contributor License Agreement (see the CLA reminder comment above)
- Merge conflicts — This PR has conflicts with the
mainbranch that need to be resolved - [WIP] status — Once ready for final review, please remove the [WIP] tag
Please address these items and @mention me for a re-review.
Automated review by github-manager-bot
ralf0131
left a comment
There was a problem hiding this comment.
Summary
This PR adds semantic convention support for the Anthropic SDK, aligning LoongSuite instrumentation with upstream GenAI semantic conventions. The changes are comprehensive (30 files, 5400+ lines).
This PR cannot be approved/merged until the following issues are resolved:
- CLA not signed — Please sign the Contributor License Agreement (see the CLA reminder comment above)
- Merge conflicts — This PR has conflicts with the main branch that need to be resolved
- [WIP] status — Once ready for final review, please remove the [WIP] tag
Please address these items and @mention me for a re-review.
Automated review by github-manager-bot
ralf0131
left a comment
There was a problem hiding this comment.
Summary
Adds Anthropic Python SDK semantic convention support for LoongSuite. This is a large implementation (5000+ lines, 30 files) covering the full instrumentation package for the Anthropic SDK, including message/tool-use span generation, token usage accounting, and streaming support.
This PR is marked as [WIP] and the contributor has not yet signed the CLA (see the CLA reminder comment above). Sharing initial observations:
Findings
- [Info] Comprehensive implementation covering the full Anthropic SDK surface area (messages, tool use, streaming).
- [Info] Follows the established LoongSuite instrumentation package structure — consistent with other packages.
- [Info] PR description still has the default template placeholders — consider filling in details before marking ready.
- [Info]
⚠️ CLA not signed — this PR cannot be merged until the contributor signs the CLA via cla-assistant.io.
Suggestions
- Fill in the PR description with test details and a summary of changes
- Sign the CLA to unblock merge when ready
- Consider adding a CHANGELOG entry
Automated review by github-manager-bot
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.