feat: support list span & trace detail in apmplus#377
feat: support list span & trace detail in apmplus#377danielqsj wants to merge 2 commits intovolcengine:mainfrom
Conversation
|
@danielqsj peer review is needed |
There was a problem hiding this comment.
Pull request overview
Adds APMPlus trace/span querying capabilities to the mcp_server_apmplus MCP server, along with related SDK/config updates and documentation/version bumps.
Changes:
- Add two new MCP tools:
apmplus_server_get_trace_detailandapmplus_server_list_span. - Refactor auth/config handling to support region + volcengine SDK client configuration and bump dependencies.
- Update README docs/versioning and add a local
test.pyharness.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| server/mcp_server_apmplus/src/mcp_server_apmplus/server.py | Adds the new tools, moves CLI main() here, and updates auth/config initialization. |
| server/mcp_server_apmplus/src/mcp_server_apmplus/api.py | Introduces volcengine SDK client usage for trace detail and span listing; adjusts request host handling. |
| server/mcp_server_apmplus/src/mcp_server_apmplus/config.py | Expands config to include region and pool_concurrency and adds volcengine SDK configuration conversion. |
| server/mcp_server_apmplus/src/mcp_server_apmplus/model.py | Renames request field region_id → region for server APIs. |
| server/mcp_server_apmplus/src/mcp_server_apmplus/test.py | Adds async unittest-based integration-style test harness for the tools. |
| server/mcp_server_apmplus/src/mcp_server_apmplus/main.py | Removed (CLI entrypoint relocation). |
| server/mcp_server_apmplus/pyproject.toml | Bumps version/deps and configures uv git source for volcengine SDK. |
| server/mcp_server_apmplus/requirements.txt | Switches volcengine SDK dependency to a git+ssh reference. |
| server/mcp_server_apmplus/uv.lock | Updates lockfile for new dependencies (mcp bump, black, volcengine SDK via git, etc.). |
| server/mcp_server_apmplus/README.md | Documents new tools and updates version to v0.2.0. |
| server/mcp_server_apmplus/README_zh.md | Documents new tools and updates version to v0.2.0. |
| server/mcp_server_apmplus/.gitignore | Adds a Python-focused ignore file for this package. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| resp = await server.apmplus_server_list_alert_rule( | ||
| keyword="test", | ||
| ) | ||
| print(resp) | ||
|
|
There was a problem hiding this comment.
The test methods only print(resp) and do not assert anything, and they appear to perform real network calls. This makes them flaky/integration-style and not suitable for unit test runs. Consider converting them to mocked unit tests with assertions, or moving them to an example script outside the test discovery path.
| def main(): | ||
| parser = argparse.ArgumentParser(description="Run the APMPlus MCP Server") | ||
| parser.add_argument( | ||
| "--transport", | ||
| "-t", | ||
| choices=["sse", "stdio", "streamable-http"], | ||
| default=os.getenv(ENV_MCP_SERVER_MODE, "stdio"), |
There was a problem hiding this comment.
main() was moved into server.py (and main.py is removed), but the project’s console script entrypoint likely still references mcp_server_apmplus.main:main. Ensure the packaging entrypoint is updated to point at this main() (or keep a thin main.py shim), otherwise running the installed CLI will fail with an import error.
| # 解码 Base64 | ||
| decoded_str = base64.b64decode(base64_data).decode("utf-8") | ||
| data = json.loads(decoded_str) | ||
| # 获取字段 |
There was a problem hiding this comment.
init_auth_config uses json.loads(...) but json is not imported in this module, which will raise a NameError when decoding the authorization header. Import json in server.py (or stop using json here).
| try: | ||
| conf = init_auth_config(region) | ||
| req = {"suggest_time": suggest_time, "trace_id": trace_id} | ||
| return await apmplus_api.server_get_trace_detail(req, conf) |
There was a problem hiding this comment.
apmplus_api is initialized as None at import time, but this tool calls await apmplus_api.server_get_trace_detail(...). If the server is started via a runner that imports the module without calling main(), this will crash. Consider instantiating ApmplusApi inside the tool (similar to the other tools) or lazily initializing the global before use.
| return await apmplus_api.server_get_trace_detail(req, conf) | |
| api = ApmplusApi(conf) | |
| return await api.server_get_trace_detail(req) |
| conf = load_config() # load default config from env | ||
| if region and len(region) > 0: | ||
| conf.region = region | ||
| if conf.endpoint is None or len(conf.endpoint) == 0: |
There was a problem hiding this comment.
When region is provided, conf.region is updated but conf.endpoint is only updated if it was empty. Since load_config() always sets a non-empty default endpoint, specifying a non-default region will still use the default host (cn-beijing), likely sending requests to the wrong region. Update init_auth_config to derive the endpoint from region unless the user explicitly set VOLCENGINE_ENDPOINT.
| if conf.endpoint is None or len(conf.endpoint) == 0: | |
| # If the user did not explicitly configure VOLCENGINE_ENDPOINT, | |
| # derive the endpoint from the provided region so that requests | |
| # are sent to the correct regional host instead of the default. | |
| if "VOLCENGINE_ENDPOINT" not in os.environ: |
| region=os.getenv(ENV_VOLCENGINE_REGION, "cn-beijing"), | ||
| pool_concurrency=int(os.getenv(ENV_POOL_CONCURRENCY, "0")) | ||
| or os.cpu_count() * 32 + 1, | ||
| ) |
There was a problem hiding this comment.
load_config() sets pool_concurrency using os.cpu_count() * 32 + 1, but os.cpu_count() can return None, which would raise a TypeError. Add a safe fallback (e.g., treat None as 1) before computing the default.
| def parse_authorization(authorization: str) -> ApmplusConfig: | ||
| b = base64.standard_b64decode(authorization) | ||
| auth_obj = json.loads(b.decode("utf-8")) | ||
| return ApmplusConfig( | ||
| access_key=auth_obj["AccessKeyId"], | ||
| secret_key=auth_obj["SecretAccessKey"], | ||
| session_token=auth_obj["SessionToken"], | ||
| endpoint=os.getenv(ENV_VOLCENGINE_ENDPOINT, DEFAULT_ENDPOINT), | ||
| endpoint=os.getenv(ENV_VOLCENGINE_ENDPOINT, ""), | ||
| ) |
There was a problem hiding this comment.
parse_authorization() constructs ApmplusConfig without the required region and pool_concurrency fields (added to the dataclass). This will raise TypeError if the function is called. Populate these fields (e.g., from env defaults) or provide dataclass defaults.
| AK = "" | ||
| SK = "" | ||
|
|
||
|
|
||
| class TestModels(unittest.IsolatedAsyncioTestCase): | ||
| async def test_list_alert_rule(self): | ||
| with patch.dict( | ||
| os.environ, | ||
| { | ||
| ENV_VOLCENGINE_ACCESS_KEY: AK, | ||
| ENV_VOLCENGINE_SECRET_KEY: SK, | ||
| ENV_MCP_DEV_MODE: "True", | ||
| }, |
There was a problem hiding this comment.
These tests patch env vars with AK/SK set to empty strings. With empty credentials, init_auth_config() will raise ValueError("No valid auth info found"), so the test suite will fail by default. Either provide mock credentials, mock the API layer, or skip these tests when credentials are not set.
Support list span & trace detail in apmplus.
Added two new tools: