feat: add telemetry support for query execution tracking and configur… #1900
Conversation
…ith dynamic URL handling
…ent with OAuth support
…ncing model organization and retrieval
…alog components with icons for vision and function call
…nd binding Space accounts
…ssword management and improved UI elements
…meToggle components
…for PostgreSQL compatibility
… and update related logic across the application
…ching mechanism in UserService
…refactor UserService to utilize new service methods
…ModelsDialog and EmbeddingForm components
…ent Space model synchronization in ModelManager
…tion error handling
…d clean up ModelsDialog constants
…components using Suspense
…provider deletion confirmation UI
…e availability and update related components
There was a problem hiding this comment.
Pull request overview
This PR adds telemetry support to track query execution metrics and send them to a configurable remote server. The implementation collects runtime statistics including adapter type, runner name, model name, execution duration, errors, and version information, then asynchronously uploads this data to a Space server.
- Added telemetry configuration section with enabled flag and server URL
- Implemented async telemetry reporting module with payload sanitization and error handling
- Integrated telemetry collection into the chat query handler to track execution metrics
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| src/langbot/templates/config.yaml | Added telemetry configuration section with enabled flag and server URL settings |
| src/langbot/pkg/telemetry/telemetry.py | New module implementing async telemetry payload sending with sanitization and timeout handling |
| src/langbot/pkg/telemetry/init.py | Empty init file for telemetry package |
| src/langbot/pkg/pipeline/process/handlers/chat.py | Integrated telemetry tracking into query handler to collect execution metrics in finally block |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if 'start_ts' in locals(): | ||
| duration_ms = int((end_ts - start_ts) * 1000) |
There was a problem hiding this comment.
Using locals() to check for variable existence is fragile and can lead to unexpected behavior. The start_ts variable is always defined on line 92 before any exception can occur, so this check using 'start_ts' in locals() is unnecessary. If there's concern about exceptions happening before line 92, the code should use a different pattern such as initializing start_ts to None before the try block and checking if start_ts is not None.
| 'model_name': model_name, | ||
| 'version': semantic_version, | ||
| 'pipeline_plugins': pipeline_plugins, | ||
| 'error': locals().get('error_info', None), |
There was a problem hiding this comment.
Using locals().get('error_info', None) to retrieve error_info is fragile and indirect. The error_info variable is only set within the exception handler on line 150, so it won't exist in the finally block unless an exception occurred. A clearer approach would be to initialize error_info = None before the try block and set it in the except block, making the code more explicit and maintainable.
| - timeout_seconds: optional int, overall request timeout (default 5) | ||
| Posts to {server.rstrip('/')}/api/v1/telemetry as JSON. Failures are logged but do not raise. |
There was a problem hiding this comment.
The docstring mentions a timeout_seconds configuration parameter that doesn't appear to be used in the implementation. The actual code uses a hardcoded timeout of 10 seconds on lines 52 and 55. Either implement support for the documented timeout_seconds parameter or remove it from the docstring to avoid confusion.
| print('Exception during telemetry send:') | ||
| import traceback | ||
| traceback.print_exc() | ||
| except Exception as e: | ||
| try: | ||
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | ||
| except Exception: | ||
| pass | ||
| print('Exception while creating HTTP client for telemetry:') | ||
| import traceback | ||
| traceback.print_exc() | ||
| except Exception as e: | ||
| # Never raise from telemetry; surface as warning for visibility | ||
| try: | ||
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | ||
| except Exception: | ||
| pass | ||
| print('Unexpected telemetry error:') | ||
| import traceback | ||
| traceback.print_exc() |
There was a problem hiding this comment.
The print statements and traceback.print_exc() calls should be removed or replaced with proper logging. These debug print statements are inappropriate for production code and will clutter stdout/stderr. The exception is already being logged with exc_info=True on line 83, which provides the same traceback information through the proper logging system.
| print('Exception during telemetry send:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| try: | |
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| print('Exception while creating HTTP client for telemetry:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| # Never raise from telemetry; surface as warning for visibility | |
| try: | |
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| print('Unexpected telemetry error:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| try: | |
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| except Exception as e: | |
| # Never raise from telemetry; surface as warning for visibility | |
| try: | |
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | |
| except Exception: | |
| pass |
| print('Exception during telemetry send:') | ||
| import traceback | ||
| traceback.print_exc() | ||
| except Exception as e: | ||
| try: | ||
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | ||
| except Exception: | ||
| pass | ||
| print('Exception while creating HTTP client for telemetry:') | ||
| import traceback | ||
| traceback.print_exc() | ||
| except Exception as e: | ||
| # Never raise from telemetry; surface as warning for visibility | ||
| try: | ||
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | ||
| except Exception: | ||
| pass | ||
| print('Unexpected telemetry error:') | ||
| import traceback | ||
| traceback.print_exc() |
There was a problem hiding this comment.
The print statements and traceback.print_exc() calls should be removed or replaced with proper logging. These debug print statements are inappropriate for production code and will clutter stdout/stderr. The exception is already being logged with exc_info=True on line 92, which provides the same traceback information through the proper logging system.
| print('Exception during telemetry send:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| try: | |
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| print('Exception while creating HTTP client for telemetry:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| # Never raise from telemetry; surface as warning for visibility | |
| try: | |
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| print('Unexpected telemetry error:') | |
| import traceback | |
| traceback.print_exc() | |
| except Exception as e: | |
| try: | |
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | |
| except Exception: | |
| pass | |
| except Exception as e: | |
| # Never raise from telemetry; surface as warning for visibility | |
| try: | |
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | |
| except Exception: | |
| pass |
| async with httpx.AsyncClient(timeout=httpx.Timeout(10)) as client: | ||
| try: | ||
| # Use asyncio.wait_for to ensure we always bound the total time | ||
| resp = await asyncio.wait_for(client.post(url, json=sanitized), timeout=10 + 1) |
There was a problem hiding this comment.
There is redundant timeout handling here. The timeout value of 10 seconds is specified twice - once in the httpx.Timeout(10) and again in asyncio.wait_for with timeout=10+1. This creates confusion and the asyncio.wait_for timeout should align with the httpx timeout or use a configuration value. Additionally, the hardcoded timeout value of 10 seconds should be extracted as a configurable parameter or at least a named constant for maintainability.
|
|
||
| import asyncio | ||
| import httpx | ||
| import typing |
There was a problem hiding this comment.
Import of 'typing' is not used.
| import typing |
| if isinstance(j, dict) and j.get('code') is not None and int(j.get('code')) >= 400: | ||
| app_err = True | ||
| ap.logger.warning(f'Telemetry post to {url} returned application error code {j.get("code")} - {j.get("msg")}') | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # Ignore JSON parsing / shape errors; treat as no structured app-level error |
| except Exception as e: | ||
| try: | ||
| ap.logger.warning(f'Failed to create HTTP client for telemetry or sanitize payload: {e}', exc_info=True) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| # Never raise from telemetry; surface as warning for visibility | ||
| try: | ||
| ap.logger.warning(f'Unexpected telemetry error: {e}', exc_info=True) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| except Exception: | |
| # If logging itself fails, ignore the error to avoid raising from telemetry. |
…r feedback in authentication flows
…lugin market, and roadmap across multiple languages
…ustomApiError for improved error messaging
…layout consistency
…ng Space for account authentication
refactor: model config dialog and introduce LangBot Models service integration
…etry' and adjust related parameters
…etry' and adjust related parameters
…ation
概述 / Overview
增加了遥测数据上传，将使用运行器，模型，运行时间和适配器及使用版本和错误上传
更改前后对比截图 / Screenshots
检查清单 / Checklist
PR 作者完成 / For PR author
请在方括号间写
x以打勾 / Please tick the box withx项目维护者完成 / For project maintainer