Refactor loongsuite instrumentation for langchain#133
Refactor loongsuite instrumentation for langchain#133Cirilla-zmh wants to merge 14 commits intoalibaba:mainfrom
Conversation
PR #133 代码审查结果✅ 符合规范的项目
|
Cirilla-zmh
left a comment
There was a problem hiding this comment.
PR #133 代码审查结果
✅ 符合规范的项目
- pyproject.toml 配置正确 - 与其他同类 instrumentation 一致
- 包含 version 文件 - 内容为
0.2.0.dev,符合开发版本规范 - 包含 package 文件 - 正确设置了 langchain_core 依赖版本
- LICENSE 文件头完整 - 所有文件都包含正确的 Apache 2.0 LICENSE 头
- CHANGELOG 已更新 - 包含了 0.1.0 版本的初始化记录
- README.md 已更新 - 提供了完整的安装和使用说明
- GitHub Workflow 文件已生成 -
loongsuite_lint_0.yml和loongsuite_test_0.yml已创建
⚠️ 需要修复的问题
1. tox-loongsuite.ini 中 langchain 测试环境被注释
在 tox-loongsuite.ini 文件中,langchain 相关的测试环境被注释掉了(行 39-41):
; ; loongsuite-instrumentation-langchain
; py3{9,10,11,12,13}-test-loongsuite-instrumentation-langchain
; lint-loongsuite-instrumentation-langchain同时相关的依赖配置(行 100)和命令配置(行 150-151)也存在但被注释。
这会导致 GitHub Actions 中的测试不会运行,需要取消注释。
2. util/opentelemetry-util-genai/handler.py 缺少 LoongSuite Extension 注释
根据审查规范,修改 util/opentelemetry-util-handler 中的特定文件(如 handler.py)时,需要在改动处添加 "LoongSuite Extension" 注释。但当前 handler.py 文件中没有看到相关注释。
3. 确认没有多余的 LICENSE 文件
需要确认整个 PR 中没有在 instrumentation 目录下添加不必要的 LICENSE 文件。
🔍 潜在资源泄漏风险检查
从代码结构来看:
- 使用了
wrapt.wrap_function_wrapper进行函数包装,这是安全的做法 - CallbackManager 的处理逻辑看起来合理,通过
add_handler添加 tracer - 没有明显的上下文泄漏或内存泄漏风险
📋 建议
- 取消 tox-loongsuite.ini 中 langchain 测试环境的注释(行 39-41, 100, 150-151)
- 在 util/opentelemetry-util-genai/handler.py 的修改处添加 "LoongSuite Extension" 注释
- 确认没有多余的 LICENSE 文件
🎯 总体评估
PR #133 基本符合 LoongSuite instrumentation 规范,主要问题在于测试配置和注释缺失。建议作者修复上述问题后再合并。
审查结论:需要修改后重新提交
详细审查反馈 - 具体修改位置🔧 需要修复的具体问题1. tox-loongsuite.ini 中 langchain 测试环境被注释文件: 需要取消注释以下行:
2. util/opentelemetry-util-genai/handler.py 缺少 LoongSuite Extension 注释文件: 3. 确认没有多余的 LICENSE 文件请检查整个 PR 变更,确保没有在 instrumentation 目录中添加不必要的 LICENSE 文件。 其他方面都符合规范! 修复以上问题后就可以批准合并了。 |
Change-Id: Id2d3e8f987ae2cf91c1c1f0bc1187124b473a8a9 Co-developed-by: Cursor <noreply@cursor.com>
…ration - Added `opentelemetry-util-genai` as a dependency. - Integrated `ExtendedTelemetryHandler` into `LangChainInstrumentor` for improved telemetry handling. - Updated `LoongsuiteTracer` to utilize the new handler for span creation and context management. - Refactored utility functions for better data extraction and base64 image filtering. - Added tests for agent span detection and base64 filtering functionality. This update enhances the data extraction capabilities and improves the overall telemetry experience in LangChain. Change-Id: Ib45f3cc60e6169eed5ed8c47683be4f8deb3daec Co-developed-by: Cursor <noreply@cursor.com>
…ure improvements - Updated `LoongsuiteTracer` to explicitly manage context propagation for parent-child span relationships. - Introduced new utility functions for checking content capture settings in chain spans. - Enhanced tests to verify input/output content capture for chains, LLMs, tools, and retrievers. - Added support for capturing message content in span attributes based on configuration. These changes improve the accuracy and reliability of telemetry data in LangChain, ensuring better tracking of operations and their contexts. Change-Id: Ib45f3cc60e6169eed5ed8c47683be4f8deb3daec Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: I83b3e6d0d09b3dd083fcec97c75afa0d89d5c792 Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: I34b48596cff260f4701373edf619626359c54319 Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: I1d31136adc5e0509e0572eaf4dbda4014f8ad0ce Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: Ibf2cddc6b0f2c2ef224c2749ae29729bc60bb590 Co-developed-by: Cursor <noreply@cursor.com>
| @@ -36,9 +36,9 @@ envlist = | |||
| ; py3{9,10,11,12,13}-test-loongsuite-instrumentation-dify | |||
There was a problem hiding this comment.
注释格式不正确。应该使用 而不是 。请修正注释格式以保持一致性。
Cirilla-zmh
left a comment
There was a problem hiding this comment.
感谢提交 PR!我已经添加了两个行内评论,主要涉及:
- tox-loongsuite.ini 注释格式问题:第 39 行的注释格式应该与其他 instrumentation 保持一致
- tox-loongsuite.ini 依赖项命名一致性:第 100 行的环境名称应该使用
loongsuite-langchain-*前缀以保持一致性
请修复这些问题后,我会重新审查并批准 PR。
Change-Id: Ic63cc0128f87670baa5455dbf785f41dcca69494 Co-developed-by: Cursor <noreply@cursor.com>
…r id Change-Id: Id5acdec8ad24cef90250fbd82149773856c9569f Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: I51457e772e64f87fe7aa3bd816dc805a519b7170 Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: Ib5de526650719ff7a7867b55217c5c6f71e85740 Co-developed-by: Cursor <noreply@cursor.com>
Change-Id: Idc634b3e9c750437e0716387cad4e99cf3157f5d Co-developed-by: Cursor <noreply@cursor.com>
8630acb to
bd608f5
Compare
Change-Id: Ibfe8ce40d19b65c0d80a5a56407b2d71ff93fc17 Co-developed-by: Cursor <noreply@cursor.com>
There was a problem hiding this comment.
Pull request overview
Refactors LoongSuite LangChain instrumentation to a callback-based tracer architecture, extends opentelemetry-util-genai for explicit context propagation and safer context detachment, and adds LangGraph support for ReAct agent metadata propagation.
Changes:
- Added Entry (
ENTRY) and ReAct Step (STEP) span support (types, attribute helpers, handler APIs, tests, docs). - Introduced optional
contextparameter acrossstart_*APIs and centralized safe context detachment via_safe_detach. - Added new LangGraph instrumentation package + CI/tox matrices and a rewritten LangChain test suite.
Reviewed changes
Copilot reviewed 57 out of 58 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| util/opentelemetry-util-genai/tests/test_extended_handler.py | Adds tests for new Entry/ReAct Step handler APIs and baggage propagation. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/handler.py | Adds optional context for start_llm and introduces _safe_detach. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/extended_metrics.py | Extends metrics recorder typing to include Entry/ReAct Step invocations. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/extended_handler.py | Adds Entry/ReAct Step span lifecycle methods + context propagation across operations. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_multimodal_processing.py | Switches detach to _safe_detach in async multimodal pipeline. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_extended_semconv/gen_ai_extended_attributes.py | Adds semantic convention constants + new span kinds/operation names. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_extended_common/common_utils.py | Implements attribute application for Entry and ReAct Step spans. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_extended_common/common_types.py | Introduces EntryInvocation and ReactStepInvocation dataclasses. |
| util/opentelemetry-util-genai/src/opentelemetry/util/genai/_extended_common/init.py | Exposes new invocation types and helpers as a package API. |
| util/opentelemetry-util-genai/README-loongsuite.rst | Documents Entry and ReAct Step operations and baggage “traffic coloring”. |
| util/opentelemetry-util-genai/CHANGELOG-loongsuite.md | Changelog entries for Entry/ReAct Step + context param + _safe_detach change. |
| tox-loongsuite.ini | Adds langchain/langgraph oldest/latest env matrices and lint envs. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/test_react_step_spans.py | Integration tests for LangGraph ReAct → LangChain tracer → OTel spans. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/test_patch.py | Validates create_react_agent and Pregel.stream patching. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/test_instrumentor.py | Adds basic lifecycle tests for the LangGraph instrumentor. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/requirements.oldest.txt | Adds oldest dependency set for LangGraph instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/requirements.latest.txt | Adds latest dependency set for LangGraph instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/conftest.py | Shared OTel providers/fixtures and dual instrumentor setup for integration tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/tests/init.py | Adds package marker for LangGraph instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/version.py | Introduces new package version module. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/package.py | Declares instrumented dependency and metrics support. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/internal/patch.py | Implements wrapt-based patching for ReAct metadata injection. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/internal/init.py | Adds package marker for internal patch module. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/init.py | Implements LangGraphInstrumentor instrument/uninstrument logic. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/pyproject.toml | Adds LangGraph instrumentation packaging metadata and entry point. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/README.md | Documents LangGraph instrumentation behavior and integration data flow. |
| instrumentation-loongsuite/loongsuite-instrumentation-langgraph/CHANGELOG.md | Adds initial changelog for the new LangGraph instrumentation package. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_tool_spans.py | Adds unit tests for tool span creation and content capture gating. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_retriever_spans.py | Adds unit tests for retriever span creation and content capture gating. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_react_step_patch.py | Tests AgentExecutor ReAct step patching and span creation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_llm_spans.py | Adds LLM span tests (attrs/content capture/token usage/error status). |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_instrumentor.py | Adds lifecycle tests for wrapping/unwrapping callback manager init. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_data_extraction.py | Adds unit tests for new run/message extraction helpers. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_chain_spans.py | Adds chain span tests including input/output capture gating. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_basic.py | Adds end-to-end basic integration tests for span creation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/test_agent_spans.py | Adds agent run-type detection tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/requirements.oldest.txt | Adds oldest dependency set for LangChain instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/requirements.latest.txt | Adds latest dependency set for LangChain instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_token_counts.py | Removes legacy token counting tests tied to old implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_singleton_tracer.py | Removes legacy singleton tracer tests tied to old implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_prompts.py | Removes legacy prompt parsing tests tied to old implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_metadata.py | Removes legacy metadata tests tied to old implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_message_parsing.py | Removes legacy message parsing tests tied to old implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/instrumentation/langchain/test_langchain_instrumentor.py | Removes legacy large integration test suite (old wrapt-based approach). |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/conftest.py | Adds standardized pytest fixtures for providers/exporters and content modes. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/tests/init.py | Adds package marker for LangChain instrumentation tests. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/src/opentelemetry/instrumentation/langchain/internal/semconv.py | Simplifies semconv to re-export util-genai semconv constants. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/src/opentelemetry/instrumentation/langchain/internal/patch.py | Adds AgentExecutor step wrapper implementation. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/src/opentelemetry/instrumentation/langchain/internal/_utils.py | Rewrites extraction utilities and adds LangGraph/agent detection helpers. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/src/opentelemetry/instrumentation/langchain/init.py | Rewrites instrumentor lifecycle, adds AgentExecutor patching + unwrap on uninstrument. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/pyproject.toml | Updates packaging/deps to include util-genai and align supported versions. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/README.md | Updates installation and usage docs + traced operations matrix. |
| instrumentation-loongsuite/loongsuite-instrumentation-langchain/CHANGELOG.md | Adds changelog for rewrite, ReAct Step, and LangGraph agent support. |
| copilot-instructions.md | Adds style guidance for PLC0415 inline imports and naming conventions. |
| .github/workflows/loongsuite_test_0.yml | Adds CI jobs for langchain/langgraph oldest/latest across Python versions. |
| .github/workflows/loongsuite_lint_0.yml | Adds CI lint jobs for langchain/langgraph instrumentations. |
Comments suppressed due to low confidence (6)
util/opentelemetry-util-genai/src/opentelemetry/util/genai/_extended_common/common_utils.py:1
invocation.attributesis applied last, which allows user-provided keys to overwrite required semantic convention fields likegen_ai.operation.nameandgen_ai.span.kind. To keep spans semconv-compliant, consider either (a) applyinginvocation.attributesearlier and writing required attributes last, or (b) filtering out collisions (skip/rename user keys that overlap required keys).
util/opentelemetry-util-genai/src/opentelemetry/util/genai/handler.py:1_RUNTIME_CONTEXTis a private OpenTelemetry API and may change without notice across OTel releases. If the goal is to avoid the SDK's ERROR log, consider using only public APIs and suppressing/handling the exception at the call site (or switching to a documented hook/flag if one exists), so util-genai doesn’t depend on private internals.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/handler.py:1_RUNTIME_CONTEXTis a private OpenTelemetry API and may change without notice across OTel releases. If the goal is to avoid the SDK's ERROR log, consider using only public APIs and suppressing/handling the exception at the call site (or switching to a documented hook/flag if one exists), so util-genai doesn’t depend on private internals.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/handler.py:1_RUNTIME_CONTEXTis a private OpenTelemetry API and may change without notice across OTel releases. If the goal is to avoid the SDK's ERROR log, consider using only public APIs and suppressing/handling the exception at the call site (or switching to a documented hook/flag if one exists), so util-genai doesn’t depend on private internals.
instrumentation-loongsuite/loongsuite-instrumentation-langgraph/src/opentelemetry/instrumentation/langgraph/init.py:1- The project name appears as “LongSuite” here, while the repository/package naming elsewhere uses “LoongSuite”. Consider standardizing this string to “LoongSuite” for consistency across docs and module docstrings.
util/opentelemetry-util-genai/src/opentelemetry/util/genai/extended_handler.py:1 - These are two separate imports from the same module and can be consolidated into a single import block. This reduces import noise and avoids triggering “reimported” style/lint rules in some configurations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _extract_token_usage(run: Any) -> tuple[int | None, int | None]: | ||
| """Return (input_tokens, output_tokens) from a completed LLM Run.""" | ||
| outputs = getattr(run, "outputs", None) or {} | ||
| llm_output = outputs.get("llm_output") or {} | ||
| token_usage = ( | ||
| llm_output.get("token_usage") or llm_output.get("usage") or {} | ||
| ) | ||
|
|
||
| input_tokens = token_usage.get("prompt_tokens") or token_usage.get( | ||
| "input_tokens" | ||
| ) | ||
| return is_true_value(capture_content) | ||
| output_tokens = token_usage.get("completion_tokens") or token_usage.get( | ||
| "output_tokens" | ||
| ) | ||
| return ( | ||
| int(input_tokens) if input_tokens is not None else None, | ||
| int(output_tokens) if output_tokens is not None else None, | ||
| ) |
There was a problem hiding this comment.
The new _extract_token_usage only reads outputs["llm_output"].token_usage/usage and does not handle other common LangChain formats (e.g., generations[0][0].generation_info["token_usage"] or generations[0][0].message.response_metadata["token_usage"]). This will cause token usage attributes/metrics to be missing for providers that don't populate llm_output. Consider restoring the prior fallback logic and adding focused unit tests covering these formats.
|
|
||
| ## Requirements | ||
|
|
||
| - Python >= 3.8 |
There was a problem hiding this comment.
The README states Python >= 3.8, but pyproject.toml for loongsuite-instrumentation-langchain requires >=3.9. Please align the README requirement with the package metadata to avoid user confusion.
| - Python >= 3.8 | |
| - Python >= 3.9 |
Description
Summary
Rewrites
loongsuite-instrumentation-langchainfrom scratch, replacing the legacywrapt-based function wrapping with a BaseTracer callback approach and integratingopentelemetry-util-genaifor standardized GenAI semantic convention compliance.Design
Tracer architecture —
LoongsuiteTracerextendslangchain_core.tracers.base.BaseTracerand hooks into the fine-grained_on_*_start/_on_*_end/_on_*_errorcallbacks. This lets us extract telemetry at the point where LangChain's Run objects are fully populated, rather than monkey-patching individual methods.Operation type mapping — Each LangChain run type is mapped to the appropriate
util-genaihandler method:start_llm/stop_llm/fail_llmstart_invoke_agent/stop_invoke_agent/fail_invoke_agentCHAINspan kindstart_execute_tool/stop_execute_tool/fail_execute_toolstart_retrieve/stop_retrieve/fail_retrieveAgent runs are distinguished from generic chains by
run.name(e.g.AgentExecutor,MRKLChain).Context propagation — Follows the Robin/OpenLLMetry pattern: parent-child span relationships are established by passing Context explicitly to
start_span/handler.start_*, avoiding hazardousattach/detachin a callback system where exceptions between callbacks can leak context. The sole exception is generic Chain spans, which doattach/detachso that non-LangChain child operations (e.g. HTTP calls) nest correctly.Content capture gating — Chain
input.value/output.valueattributes are gated behindutil-genai'sis_experimental_mode()andget_content_capturing_mode(), consistent with how LLM/Tool/Retriever content is already controlled.Thread safety — All access to the per-run bookkeeping dict is protected by
RLock.TTFT —
on_llm_new_tokenrecords the monotonic first-token timestamp;util-genaicomputesgen_ai.response.time_to_first_tokenon span finalization.Changes to util-genai
start_*methods inhandler.pyandextended_handler.pyaccept an optionalcontextparameter, forwarded tostart_spanfor explicit parent-child linking._safe_detachuses_RUNTIME_CONTEXT.detachdirectly to avoid the noisy ERROR log from the OTel SDK'scontext_api.detachwrapper.otel_contextreference in_multimodal_processing.py.##Test plan
tox-loongsuite.iniconfigured witholdest/latestdependency matrices.ERROR-level log output during test execution.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.