From e5039c8accac7f23628632643e0e521f474e2452 Mon Sep 17 00:00:00 2001 From: Justin Barnett Date: Thu, 4 Sep 2025 12:09:34 -0400 Subject: [PATCH 01/21] added optional telemetry --- README.md | 12 + TELEMETRY.md | 178 +++++++++++ .../Editor/Helpers/TelemetryHelper.cs | 188 ++++++++++++ .../Editor/Helpers/TelemetryHelper.cs.meta | 11 + UnityMcpBridge/Editor/MCPForUnityBridge.cs | 6 + UnityMcpBridge/UnityMcpServer~/src/config.py | 4 + UnityMcpBridge/UnityMcpServer~/src/server.py | 28 ++ .../UnityMcpServer~/src/telemetry.py | 289 ++++++++++++++++++ .../src/telemetry_decorator.py | 42 +++ .../UnityMcpServer~/src/test_telemetry.py | 160 ++++++++++ .../src/tools/manage_script.py | 11 + 11 files changed, 929 insertions(+) create mode 100644 TELEMETRY.md create mode 100644 UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs create mode 100644 UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta create mode 100644 UnityMcpBridge/UnityMcpServer~/src/telemetry.py create mode 100644 UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py create mode 100644 UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py diff --git a/README.md b/README.md index c3082f741..338a04e07 100644 --- a/README.md +++ b/README.md @@ -331,6 +331,18 @@ Help make MCP for Unity better! --- +## ๐Ÿ“Š Telemetry & Privacy + +Unity MCP includes **privacy-focused, anonymous telemetry** to help us improve the product. We collect usage analytics and performance data, but **never** your code, project names, or personal information. + +- **๐Ÿ”’ Anonymous**: Random UUIDs only, no personal data +- **๐Ÿšซ Easy opt-out**: Set `DISABLE_TELEMETRY=true` environment variable +- **๐Ÿ“– Transparent**: See [TELEMETRY.md](TELEMETRY.md) for full details + +Your privacy matters to us. All telemetry is optional and designed to respect your workflow. + +--- + ## Troubleshooting โ“
diff --git a/TELEMETRY.md b/TELEMETRY.md new file mode 100644 index 000000000..bd8d99b9a --- /dev/null +++ b/TELEMETRY.md @@ -0,0 +1,178 @@ +# Unity MCP Telemetry + +Unity MCP includes privacy-focused, anonymous telemetry to help us improve the product. This document explains what data is collected, how to opt out, and our privacy practices. + +## ๐Ÿ”’ Privacy First + +- **Anonymous**: We use randomly generated UUIDs - no personal information +- **Non-blocking**: Telemetry never interferes with your Unity workflow +- **Easy opt-out**: Simple environment variable or Unity Editor setting +- **Transparent**: All collected data types are documented here + +## ๐Ÿ“Š What We Collect + +### Usage Analytics +- **Tool Usage**: Which MCP tools you use (manage_script, manage_scene, etc.) +- **Performance**: Execution times and success/failure rates +- **System Info**: Unity version, platform (Windows/Mac/Linux), MCP version +- **Milestones**: First-time usage events (first script creation, first tool use, etc.) + +### Technical Diagnostics +- **Connection Events**: Bridge startup/connection success/failures +- **Error Reports**: Anonymized error messages (truncated to 200 chars) +- **Server Health**: Startup time, connection latency + +### What We **DON'T** Collect +- โŒ Your code or script contents +- โŒ Project names, file names, or paths +- โŒ Personal information or identifiers +- โŒ Sensitive project data +- โŒ IP addresses (beyond what's needed for HTTP requests) + +## ๐Ÿšซ How to Opt Out + +### Method 1: Environment Variable (Recommended) +Set any of these environment variables to `true`: + +```bash +# Disable all telemetry +export DISABLE_TELEMETRY=true + +# Unity MCP specific +export UNITY_MCP_DISABLE_TELEMETRY=true + +# MCP protocol wide +export MCP_DISABLE_TELEMETRY=true +``` + +### Method 2: Unity Editor (Coming Soon) +In Unity Editor: `Window > MCP for Unity > Settings > Disable Telemetry` + +### Method 3: Manual Config +Add to your MCP client config: +```json +{ + "env": { + "DISABLE_TELEMETRY": "true" + } +} +``` + +## ๐Ÿ”ง Technical Implementation + +### Architecture +- **Python Server**: Core telemetry collection and transmission +- **Unity Bridge**: Local event collection from Unity Editor +- **Anonymous UUIDs**: Generated per-installation for aggregate analytics +- **Thread-safe**: Non-blocking background transmission +- **Fail-safe**: Errors never interrupt your workflow + +### Data Storage +Telemetry data is stored locally in: +- **Windows**: `%APPDATA%\UnityMCP\` +- **macOS**: `~/Library/Application Support/UnityMCP/` +- **Linux**: `~/.local/share/UnityMCP/` + +Files created: +- `customer_uuid.txt`: Anonymous identifier +- `milestones.json`: One-time events tracker + +### Data Transmission +- **Endpoint**: `https://telemetry.coplay.dev/unity-mcp/anonymous` +- **Method**: HTTPS POST with JSON payload +- **Retry**: Background thread with graceful failure +- **Timeout**: 10 second timeout, no retries on failure + +## ๐Ÿ“ˆ How We Use This Data + +### Product Improvement +- **Feature Usage**: Understand which tools are most/least used +- **Performance**: Identify slow operations to optimize +- **Reliability**: Track error rates and connection issues +- **Compatibility**: Ensure Unity version compatibility + +### Development Priorities +- **Roadmap**: Focus development on most-used features +- **Bug Fixes**: Prioritize fixes based on error frequency +- **Platform Support**: Allocate resources based on platform usage +- **Documentation**: Improve docs for commonly problematic areas + +### What We Don't Do +- โŒ Sell data to third parties +- โŒ Use data for advertising/marketing +- โŒ Track individual developers +- โŒ Store sensitive project information + +## ๐Ÿ› ๏ธ For Developers + +### Testing Telemetry +```bash +cd UnityMcpBridge/UnityMcpServer~/src +python test_telemetry.py +``` + +### Custom Telemetry Events +```python +from telemetry import record_telemetry, RecordType + +record_telemetry(RecordType.USAGE, { + "custom_event": "my_feature_used", + "metadata": "optional_data" +}) +``` + +### Telemetry Status Check +```python +from telemetry import is_telemetry_enabled + +if is_telemetry_enabled(): + print("Telemetry is active") +else: + print("Telemetry is disabled") +``` + +## ๐Ÿ“‹ Data Retention Policy + +- **Aggregated Data**: Retained indefinitely for product insights +- **Raw Events**: Automatically purged after 90 days +- **Personal Data**: None collected, so none to purge +- **Opt-out**: Immediate - no data sent after opting out + +## ๐Ÿค Contact & Transparency + +- **Questions**: [Discord Community](https://discord.gg/y4p8KfzrN4) +- **Issues**: [GitHub Issues](https://github.com/CoplayDev/unity-mcp/issues) +- **Privacy Concerns**: Create a GitHub issue with "Privacy" label +- **Source Code**: All telemetry code is open source in this repository + +## ๐Ÿ“Š Example Telemetry Event + +Here's what a typical telemetry event looks like: + +```json +{ + "record": "tool_execution", + "timestamp": 1704067200, + "customer_uuid": "550e8400-e29b-41d4-a716-446655440000", + "session_id": "abc123-def456-ghi789", + "version": "3.0.2", + "platform": "posix", + "data": { + "tool_name": "manage_script", + "success": true, + "duration_ms": 42.5 + } +} +``` + +Notice: +- โœ… Anonymous UUID (randomly generated) +- โœ… Tool performance metrics +- โœ… Success/failure tracking +- โŒ No code content +- โŒ No project information +- โŒ No personal data + +--- + +*Unity MCP Telemetry is designed to respect your privacy while helping us build a better tool. Thank you for helping improve Unity MCP!* \ No newline at end of file diff --git a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs new file mode 100644 index 000000000..67618e776 --- /dev/null +++ b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs @@ -0,0 +1,188 @@ +using System; +using System.Collections.Generic; +using UnityEngine; + +namespace MCPForUnity.Editor.Helpers +{ + /// + /// Unity Bridge telemetry helper for collecting usage analytics + /// Following privacy-first approach with easy opt-out mechanisms + /// + public static class TelemetryHelper + { + private const string TELEMETRY_DISABLED_KEY = "MCPForUnity.TelemetryDisabled"; + private const string CUSTOMER_UUID_KEY = "MCPForUnity.CustomerUUID"; + + /// + /// Check if telemetry is enabled (can be disabled via Environment Variable or EditorPrefs) + /// + public static bool IsEnabled + { + get + { + // Check environment variables first + var envDisable = Environment.GetEnvironmentVariable("DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(envDisable) && + (envDisable.ToLower() == "true" || envDisable == "1")) + { + return false; + } + + var unityMcpDisable = Environment.GetEnvironmentVariable("UNITY_MCP_DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(unityMcpDisable) && + (unityMcpDisable.ToLower() == "true" || unityMcpDisable == "1")) + { + return false; + } + + // Check EditorPrefs + return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); + } + } + + /// + /// Get or generate customer UUID for anonymous tracking + /// + public static string GetCustomerUUID() + { + var uuid = UnityEditor.EditorPrefs.GetString(CUSTOMER_UUID_KEY, ""); + if (string.IsNullOrEmpty(uuid)) + { + uuid = System.Guid.NewGuid().ToString(); + UnityEditor.EditorPrefs.SetString(CUSTOMER_UUID_KEY, uuid); + } + return uuid; + } + + /// + /// Disable telemetry (stored in EditorPrefs) + /// + public static void DisableTelemetry() + { + UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, true); + } + + /// + /// Enable telemetry (stored in EditorPrefs) + /// + public static void EnableTelemetry() + { + UnityEditor.EditorPrefs.SetBool(TELEMETRY_DISABLED_KEY, false); + } + + /// + /// Send telemetry data to Python server for processing + /// This is a lightweight bridge - the actual telemetry logic is in Python + /// + public static void RecordEvent(string eventType, Dictionary data = null) + { + if (!IsEnabled) + return; + + try + { + var telemetryData = new Dictionary + { + ["event_type"] = eventType, + ["timestamp"] = DateTimeOffset.UtcNow.ToUnixTimeSeconds(), + ["customer_uuid"] = GetCustomerUUID(), + ["unity_version"] = Application.unityVersion, + ["platform"] = Application.platform.ToString(), + ["source"] = "unity_bridge" + }; + + if (data != null) + { + telemetryData["data"] = data; + } + + // Send to Python server via existing bridge communication + // The Python server will handle actual telemetry transmission + SendTelemetryToPythonServer(telemetryData); + } + catch (Exception e) + { + // Never let telemetry errors interfere with functionality + if (IsDebugEnabled()) + { + Debug.LogWarning($"Telemetry error (non-blocking): {e.Message}"); + } + } + } + + /// + /// Record bridge startup event + /// + public static void RecordBridgeStartup() + { + RecordEvent("bridge_startup", new Dictionary + { + ["bridge_version"] = "3.0.2", + ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode + }); + } + + /// + /// Record bridge connection event + /// + public static void RecordBridgeConnection(bool success, string error = null) + { + var data = new Dictionary + { + ["success"] = success + }; + + if (!string.IsNullOrEmpty(error)) + { + data["error"] = error.Substring(0, Math.Min(200, error.Length)); + } + + RecordEvent("bridge_connection", data); + } + + /// + /// Record tool execution from Unity side + /// + public static void RecordToolExecution(string toolName, bool success, float durationMs, string error = null) + { + var data = new Dictionary + { + ["tool_name"] = toolName, + ["success"] = success, + ["duration_ms"] = Math.Round(durationMs, 2) + }; + + if (!string.IsNullOrEmpty(error)) + { + data["error"] = error.Substring(0, Math.Min(200, error.Length)); + } + + RecordEvent("tool_execution_unity", data); + } + + private static void SendTelemetryToPythonServer(Dictionary telemetryData) + { + // This would integrate with the existing bridge communication system + // For now, we'll just log it when debug is enabled + if (IsDebugEnabled()) + { + Debug.Log($"MCP-TELEMETRY: {telemetryData["event_type"]}"); + } + + // TODO: Integrate with MCPForUnityBridge command system + // We would send this as a special telemetry command to the Python server + } + + private static bool IsDebugEnabled() + { + try + { + return UnityEditor.EditorPrefs.GetBool("MCPForUnity.DebugLogs", false); + } + catch + { + return false; + } + } + } +} \ No newline at end of file diff --git a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta new file mode 100644 index 000000000..d7fd7b1f4 --- /dev/null +++ b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: b8f3c2d1e7a94f6c8a9b5e3d2c1a0f9e +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: \ No newline at end of file diff --git a/UnityMcpBridge/Editor/MCPForUnityBridge.cs b/UnityMcpBridge/Editor/MCPForUnityBridge.cs index 7d75908b1..69fd1b783 100644 --- a/UnityMcpBridge/Editor/MCPForUnityBridge.cs +++ b/UnityMcpBridge/Editor/MCPForUnityBridge.cs @@ -67,10 +67,16 @@ public static void StartAutoConnect() currentUnityPort = PortManager.GetPortWithFallback(); Start(); isAutoConnectMode = true; + + // Record telemetry for bridge startup + TelemetryHelper.RecordBridgeStartup(); } catch (Exception ex) { Debug.LogError($"Auto-connect failed: {ex.Message}"); + + // Record telemetry for connection failure + TelemetryHelper.RecordBridgeConnection(false, ex.Message); throw; } } diff --git a/UnityMcpBridge/UnityMcpServer~/src/config.py b/UnityMcpBridge/UnityMcpServer~/src/config.py index 5df28b8ab..d3b1021ed 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/config.py +++ b/UnityMcpBridge/UnityMcpServer~/src/config.py @@ -30,6 +30,10 @@ class ServerConfig: # Number of polite retries when Unity reports reloading # 40 ร— 250ms โ‰ˆ 10s default window reload_max_retries: int = 40 + + # Telemetry settings + telemetry_enabled: bool = True + telemetry_endpoint: str = "https://telemetry.coplay.dev/unity-mcp/anonymous" # Create a global config instance config = ServerConfig() \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index 29c7b6a78..08570bb40 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -6,6 +6,8 @@ from config import config from tools import register_all_tools from unity_connection import get_unity_connection, UnityConnection +from telemetry import record_telemetry, record_milestone, RecordType, MilestoneType +import time # Configure logging using settings from config logging.basicConfig( @@ -22,12 +24,38 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: """Handle server startup and shutdown.""" global _unity_connection logger.info("MCP for Unity Server starting up") + + # Record server startup telemetry + start_time = time.time() + record_telemetry(RecordType.STARTUP, { + "server_version": "3.0.2", + "startup_time": start_time + }) + + # Record first startup milestone + record_milestone(MilestoneType.FIRST_STARTUP) + try: _unity_connection = get_unity_connection() logger.info("Connected to Unity on startup") + + # Record successful Unity connection + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "connected", + "connection_time_ms": (time.time() - start_time) * 1000 + }) + except Exception as e: logger.warning(f"Could not connect to Unity on startup: {str(e)}") _unity_connection = None + + # Record connection failure + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "failed", + "error": str(e)[:200], + "connection_time_ms": (time.time() - start_time) * 1000 + }) + try: # Yield the connection object so it can be attached to the context # The key 'bridge' matches how tools like read_console expect to access it (ctx.bridge) diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py new file mode 100644 index 000000000..0eea57a64 --- /dev/null +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -0,0 +1,289 @@ +""" +Privacy-focused, anonymous telemetry system for Unity MCP +Inspired by Onyx's telemetry implementation with Unity-specific adaptations +""" + +import uuid +import threading +import contextvars +import json +import time +import os +import logging +from enum import Enum +from dataclasses import dataclass, asdict +from typing import Optional, Dict, Any, List +from pathlib import Path + +try: + import httpx + HAS_HTTPX = True +except ImportError: + httpx = None # type: ignore + HAS_HTTPX = False + +logger = logging.getLogger("unity-mcp-telemetry") + +class RecordType(str, Enum): + """Types of telemetry records we collect""" + VERSION = "version" + STARTUP = "startup" + USAGE = "usage" + LATENCY = "latency" + FAILURE = "failure" + TOOL_EXECUTION = "tool_execution" + UNITY_CONNECTION = "unity_connection" + CLIENT_CONNECTION = "client_connection" + +class MilestoneType(str, Enum): + """Major user journey milestones""" + FIRST_STARTUP = "first_startup" + FIRST_TOOL_USAGE = "first_tool_usage" + FIRST_SCRIPT_CREATION = "first_script_creation" + FIRST_SCENE_MODIFICATION = "first_scene_modification" + MULTIPLE_SESSIONS = "multiple_sessions" + DAILY_ACTIVE_USER = "daily_active_user" + WEEKLY_ACTIVE_USER = "weekly_active_user" + +@dataclass +class TelemetryRecord: + """Structure for telemetry data""" + record_type: RecordType + timestamp: float + customer_uuid: str + session_id: str + data: Dict[str, Any] + milestone: Optional[MilestoneType] = None + +class TelemetryConfig: + """Telemetry configuration""" + def __init__(self): + # Check environment variables for opt-out + self.enabled = not self._is_disabled() + + # Telemetry endpoint - can be configured via environment + self.endpoint = os.environ.get( + "UNITY_MCP_TELEMETRY_ENDPOINT", + "https://telemetry.coplay.dev/unity-mcp/anonymous" + ) + + # Local storage for UUID and milestones + self.data_dir = self._get_data_directory() + self.uuid_file = self.data_dir / "customer_uuid.txt" + self.milestones_file = self.data_dir / "milestones.json" + + # Request timeout + self.timeout = 10.0 + + # Session tracking + self.session_id = str(uuid.uuid4()) + + def _is_disabled(self) -> bool: + """Check if telemetry is disabled via environment variables""" + disable_vars = [ + "DISABLE_TELEMETRY", + "UNITY_MCP_DISABLE_TELEMETRY", + "MCP_DISABLE_TELEMETRY" + ] + + for var in disable_vars: + if os.environ.get(var, "").lower() in ("true", "1", "yes", "on"): + return True + return False + + def _get_data_directory(self) -> Path: + """Get directory for storing telemetry data""" + if os.name == 'nt': # Windows + base_dir = Path(os.environ.get('APPDATA', Path.home() / 'AppData' / 'Roaming')) + elif os.name == 'posix': # macOS/Linux + if 'darwin' in os.uname().sysname.lower(): # macOS + base_dir = Path.home() / 'Library' / 'Application Support' + else: # Linux + base_dir = Path(os.environ.get('XDG_DATA_HOME', Path.home() / '.local' / 'share')) + else: + base_dir = Path.home() / '.unity-mcp' + + data_dir = base_dir / 'UnityMCP' + data_dir.mkdir(parents=True, exist_ok=True) + return data_dir + +class TelemetryCollector: + """Main telemetry collection class""" + + def __init__(self): + self.config = TelemetryConfig() + self._customer_uuid: Optional[str] = None + self._milestones: Dict[str, Dict[str, Any]] = {} + self._load_persistent_data() + + def _load_persistent_data(self): + """Load UUID and milestones from disk""" + try: + # Load customer UUID + if self.config.uuid_file.exists(): + self._customer_uuid = self.config.uuid_file.read_text().strip() + else: + self._customer_uuid = str(uuid.uuid4()) + self.config.uuid_file.write_text(self._customer_uuid) + + # Load milestones + if self.config.milestones_file.exists(): + self._milestones = json.loads(self.config.milestones_file.read_text()) + except Exception as e: + logger.warning(f"Failed to load telemetry data: {e}") + self._customer_uuid = str(uuid.uuid4()) + self._milestones = {} + + def _save_milestones(self): + """Save milestones to disk""" + try: + self.config.milestones_file.write_text(json.dumps(self._milestones, indent=2)) + except Exception as e: + logger.warning(f"Failed to save milestones: {e}") + + def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: + """Record a milestone event, returns True if this is the first occurrence""" + if not self.config.enabled: + return False + + milestone_key = milestone.value + if milestone_key in self._milestones: + return False # Already recorded + + milestone_data = { + "timestamp": time.time(), + "data": data or {} + } + + self._milestones[milestone_key] = milestone_data + self._save_milestones() + + # Also send as telemetry record + self.record( + record_type=RecordType.USAGE, + data={"milestone": milestone_key, **(data or {})}, + milestone=milestone + ) + + return True + + def record(self, + record_type: RecordType, + data: Dict[str, Any], + milestone: Optional[MilestoneType] = None): + """Record a telemetry event (async, non-blocking)""" + if not self.config.enabled: + return + + if not HAS_HTTPX: + logger.debug("Telemetry disabled: httpx not available") + return + + record = TelemetryRecord( + record_type=record_type, + timestamp=time.time(), + customer_uuid=self._customer_uuid or "unknown", + session_id=self.config.session_id, + data=data, + milestone=milestone + ) + + # Send in background thread to avoid blocking + current_context = contextvars.copy_context() + thread = threading.Thread( + target=lambda: current_context.run(self._send_telemetry, record), + daemon=True + ) + thread.start() + + def _send_telemetry(self, record: TelemetryRecord): + """Send telemetry data to endpoint""" + try: + payload = { + "record": record.record_type.value, + "timestamp": record.timestamp, + "customer_uuid": record.customer_uuid, + "session_id": record.session_id, + "data": record.data, + "version": "3.0.2", # Unity MCP version + "platform": os.name + } + + if record.milestone: + payload["milestone"] = record.milestone.value + + if not httpx: + return + + with httpx.Client(timeout=self.config.timeout) as client: + response = client.post(self.config.endpoint, json=payload) + + if response.status_code == 200: + logger.debug(f"Telemetry sent: {record.record_type}") + else: + logger.debug(f"Telemetry failed: HTTP {response.status_code}") + + except Exception as e: + # Never let telemetry errors interfere with app functionality + logger.debug(f"Telemetry send failed: {e}") + +# Global telemetry instance +_telemetry_collector: Optional[TelemetryCollector] = None + +def get_telemetry() -> TelemetryCollector: + """Get the global telemetry collector instance""" + global _telemetry_collector + if _telemetry_collector is None: + _telemetry_collector = TelemetryCollector() + return _telemetry_collector + +def record_telemetry(record_type: RecordType, + data: Dict[str, Any], + milestone: Optional[MilestoneType] = None): + """Convenience function to record telemetry""" + get_telemetry().record(record_type, data, milestone) + +def record_milestone(milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: + """Convenience function to record a milestone""" + return get_telemetry().record_milestone(milestone, data) + +def record_tool_usage(tool_name: str, success: bool, duration_ms: float, error: Optional[str] = None): + """Record tool usage telemetry""" + data = { + "tool_name": tool_name, + "success": success, + "duration_ms": round(duration_ms, 2) + } + + if error: + data["error"] = str(error)[:200] # Limit error message length + + record_telemetry(RecordType.TOOL_EXECUTION, data) + +def record_latency(operation: str, duration_ms: float, metadata: Optional[Dict[str, Any]] = None): + """Record latency telemetry""" + data = { + "operation": operation, + "duration_ms": round(duration_ms, 2) + } + + if metadata: + data.update(metadata) + + record_telemetry(RecordType.LATENCY, data) + +def record_failure(component: str, error: str, metadata: Optional[Dict[str, Any]] = None): + """Record failure telemetry""" + data = { + "component": component, + "error": str(error)[:500] # Limit error message length + } + + if metadata: + data.update(metadata) + + record_telemetry(RecordType.FAILURE, data) + +def is_telemetry_enabled() -> bool: + """Check if telemetry is enabled""" + return get_telemetry().config.enabled \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py new file mode 100644 index 000000000..8e1347a3d --- /dev/null +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py @@ -0,0 +1,42 @@ +""" +Telemetry decorator for Unity MCP tools +""" + +import functools +import time +from typing import Callable, Any +from telemetry import record_tool_usage, record_milestone, MilestoneType + +def telemetry_tool(tool_name: str): + """Decorator to add telemetry tracking to MCP tools""" + def decorator(func: Callable) -> Callable: + @functools.wraps(func) + def wrapper(*args, **kwargs) -> Any: + start_time = time.time() + success = False + error = None + + try: + result = func(*args, **kwargs) + success = True + + # Record tool-specific milestones + if tool_name == "manage_script" and kwargs.get("action") == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + + # Record general first tool usage + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + + return result + + except Exception as e: + error = str(e) + raise + finally: + duration_ms = (time.time() - start_time) * 1000 + record_tool_usage(tool_name, success, duration_ms, error) + + return wrapper + return decorator \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py new file mode 100644 index 000000000..850f3b912 --- /dev/null +++ b/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py @@ -0,0 +1,160 @@ +#!/usr/bin/env python3 +""" +Test script for Unity MCP Telemetry System +Run this to verify telemetry is working correctly +""" + +import os +import time +import sys +from pathlib import Path + +# Add src to Python path for imports +sys.path.insert(0, str(Path(__file__).parent)) + +def test_telemetry_basic(): + """Test basic telemetry functionality""" + print("๐Ÿงช Testing Unity MCP Telemetry System...") + + try: + from telemetry import ( + get_telemetry, record_telemetry, record_milestone, + RecordType, MilestoneType, is_telemetry_enabled + ) + print("โœ… Telemetry module imported successfully") + except ImportError as e: + print(f"โŒ Failed to import telemetry module: {e}") + return False + + # Test telemetry enabled status + print(f"๐Ÿ“Š Telemetry enabled: {is_telemetry_enabled()}") + + # Test basic record + try: + record_telemetry(RecordType.VERSION, { + "version": "3.0.2", + "test_run": True + }) + print("โœ… Basic telemetry record sent") + except Exception as e: + print(f"โŒ Failed to send basic telemetry: {e}") + return False + + # Test milestone recording + try: + is_first = record_milestone(MilestoneType.FIRST_STARTUP, { + "test_mode": True + }) + print(f"โœ… Milestone recorded (first time: {is_first})") + except Exception as e: + print(f"โŒ Failed to record milestone: {e}") + return False + + # Test telemetry collector + try: + collector = get_telemetry() + print(f"โœ… Telemetry collector initialized (UUID: {collector._customer_uuid[:8]}...)") + except Exception as e: + print(f"โŒ Failed to get telemetry collector: {e}") + return False + + return True + +def test_telemetry_disabled(): + """Test telemetry with disabled state""" + print("\n๐Ÿšซ Testing telemetry disabled state...") + + # Set environment variable to disable telemetry + os.environ["DISABLE_TELEMETRY"] = "true" + + # Re-import to get fresh config + import importlib + import telemetry + importlib.reload(telemetry) + + from telemetry import is_telemetry_enabled, record_telemetry, RecordType + + print(f"๐Ÿ“Š Telemetry enabled (should be False): {is_telemetry_enabled()}") + + if not is_telemetry_enabled(): + print("โœ… Telemetry correctly disabled via environment variable") + + # Test that records are ignored when disabled + record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"}) + print("โœ… Telemetry record ignored when disabled") + + return True + else: + print("โŒ Telemetry not disabled by environment variable") + return False + +def test_data_storage(): + """Test data storage functionality""" + print("\n๐Ÿ’พ Testing data storage...") + + try: + from telemetry import get_telemetry + + collector = get_telemetry() + data_dir = collector.config.data_dir + + print(f"๐Ÿ“ Data directory: {data_dir}") + print(f"๐Ÿท๏ธ UUID file: {collector.config.uuid_file}") + print(f"๐ŸŽฏ Milestones file: {collector.config.milestones_file}") + + # Check if files exist + if collector.config.uuid_file.exists(): + print("โœ… UUID file exists") + else: + print("โ„น๏ธ UUID file will be created on first use") + + if collector.config.milestones_file.exists(): + print("โœ… Milestones file exists") + else: + print("โ„น๏ธ Milestones file will be created on first milestone") + + return True + + except Exception as e: + print(f"โŒ Data storage test failed: {e}") + return False + +def main(): + """Run all telemetry tests""" + print("๐Ÿš€ Unity MCP Telemetry Test Suite") + print("=" * 50) + + tests = [ + test_telemetry_basic, + test_data_storage, + test_telemetry_disabled, + ] + + passed = 0 + failed = 0 + + for test in tests: + try: + if test(): + passed += 1 + print("โœ… PASSED\n") + else: + failed += 1 + print("โŒ FAILED\n") + except Exception as e: + failed += 1 + print(f"โŒ FAILED with exception: {e}\n") + + print("=" * 50) + print(f"๐Ÿ“Š Test Results: {passed} passed, {failed} failed") + + if failed == 0: + print("๐ŸŽ‰ All telemetry tests passed!") + return True + else: + print(f"โš ๏ธ {failed} test(s) failed") + return False + +if __name__ == "__main__": + success = main() + sys.exit(0 if success else 1) \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index a41fb85cf..b486340cc 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -5,11 +5,22 @@ import time import os import base64 +try: + from telemetry_decorator import telemetry_tool + from telemetry import record_milestone, MilestoneType + HAS_TELEMETRY = True +except ImportError: + HAS_TELEMETRY = False + def telemetry_tool(tool_name: str): + def decorator(func): + return func + return decorator def register_manage_script_tools(mcp: FastMCP): """Register all script management tools with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_script") def manage_script( ctx: Context, action: str, From 99beca7cfe443ad5728f3e8d8d1a1ba1cc986981 Mon Sep 17 00:00:00 2001 From: Justin Barnett Date: Thu, 4 Sep 2025 14:43:41 -0400 Subject: [PATCH 02/21] endpoint adjustment --- TELEMETRY.md | 2 +- UnityMcpBridge/UnityMcpServer~/src/config.py | 2 +- UnityMcpBridge/UnityMcpServer~/src/telemetry.py | 7 ++----- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/TELEMETRY.md b/TELEMETRY.md index bd8d99b9a..f5105d6c8 100644 --- a/TELEMETRY.md +++ b/TELEMETRY.md @@ -78,7 +78,7 @@ Files created: - `milestones.json`: One-time events tracker ### Data Transmission -- **Endpoint**: `https://telemetry.coplay.dev/unity-mcp/anonymous` +- **Endpoint**: `https://api-prod.coplay.dev/telemetry/events` - **Method**: HTTPS POST with JSON payload - **Retry**: Background thread with graceful failure - **Timeout**: 10 second timeout, no retries on failure diff --git a/UnityMcpBridge/UnityMcpServer~/src/config.py b/UnityMcpBridge/UnityMcpServer~/src/config.py index a09079ae9..e41b2724f 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/config.py +++ b/UnityMcpBridge/UnityMcpServer~/src/config.py @@ -36,7 +36,7 @@ class ServerConfig: # Telemetry settings telemetry_enabled: bool = True - telemetry_endpoint: str = "https://telemetry.coplay.dev/unity-mcp/anonymous" + telemetry_endpoint: str = "https://api-prod.coplay.dev/telemetry/events" # Create a global config instance config = ServerConfig() \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 0eea57a64..216348328 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -61,11 +61,8 @@ def __init__(self): # Check environment variables for opt-out self.enabled = not self._is_disabled() - # Telemetry endpoint - can be configured via environment - self.endpoint = os.environ.get( - "UNITY_MCP_TELEMETRY_ENDPOINT", - "https://telemetry.coplay.dev/unity-mcp/anonymous" - ) + # Telemetry endpoint - hardcoded to Coplay production API + self.endpoint = "https://api-prod.coplay.dev/telemetry/events" # Local storage for UUID and milestones self.data_dir = self._get_data_directory() From f127024d01b34c987ae0690665ea603f7b4e270d Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 16:39:47 -0700 Subject: [PATCH 03/21] telemetry: enable tool_execution across tools via strict, async-aware decorator; add endpoint env override + urllib fallback; enrich OS fields; fix TelemetryHelper invocation --- .../Editor/Helpers/TelemetryHelper.cs | 2 +- .../UnityMcpServer~/src/telemetry.py | 72 +++++++++++++------ .../src/telemetry_decorator.py | 47 +++++++++--- .../src/tools/execute_menu_item.py | 5 +- .../UnityMcpServer~/src/tools/manage_asset.py | 5 +- .../src/tools/manage_editor.py | 13 +++- .../src/tools/manage_gameobject.py | 5 +- .../UnityMcpServer~/src/tools/manage_scene.py | 5 +- .../src/tools/manage_script.py | 30 ++++---- .../src/tools/manage_script_edits.py | 5 +- .../src/tools/manage_shader.py | 5 +- .../UnityMcpServer~/src/tools/read_console.py | 5 +- .../src/tools/resource_tools.py | 13 ++-- 13 files changed, 152 insertions(+), 60 deletions(-) diff --git a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs index 67618e776..079ca6f59 100644 --- a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs +++ b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs @@ -118,7 +118,7 @@ public static void RecordBridgeStartup() RecordEvent("bridge_startup", new Dictionary { ["bridge_version"] = "3.0.2", - ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode + ["auto_connect"] = MCPForUnityBridge.IsAutoConnectMode() }); } diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 216348328..b701e2a57 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -9,6 +9,8 @@ import json import time import os +import sys +import platform import logging from enum import Enum from dataclasses import dataclass, asdict @@ -61,8 +63,11 @@ def __init__(self): # Check environment variables for opt-out self.enabled = not self._is_disabled() - # Telemetry endpoint - hardcoded to Coplay production API - self.endpoint = "https://api-prod.coplay.dev/telemetry/events" + # Telemetry endpoint (Cloud Run default; override via env) + self.endpoint = os.environ.get( + "UNITY_MCP_TELEMETRY_ENDPOINT", + "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" + ) # Local storage for UUID and milestones self.data_dir = self._get_data_directory() @@ -172,9 +177,7 @@ def record(self, if not self.config.enabled: return - if not HAS_HTTPX: - logger.debug("Telemetry disabled: httpx not available") - return + # Allow fallback sender when httpx is unavailable (no early return) record = TelemetryRecord( record_type=record_type, @@ -196,34 +199,63 @@ def record(self, def _send_telemetry(self, record: TelemetryRecord): """Send telemetry data to endpoint""" try: + # System fingerprint (top-level remains concise; details stored in data JSON) + _platform = platform.system() # 'Darwin' | 'Linux' | 'Windows' + _source = sys.platform # 'darwin' | 'linux' | 'win32' + _platform_detail = f"{_platform} {platform.release()} ({platform.machine()})" + _python_version = platform.python_version() + + # Enrich data JSON so BigQuery stores detailed fields without schema change + enriched_data = dict(record.data or {}) + enriched_data.setdefault("platform_detail", _platform_detail) + enriched_data.setdefault("python_version", _python_version) + payload = { "record": record.record_type.value, "timestamp": record.timestamp, "customer_uuid": record.customer_uuid, "session_id": record.session_id, - "data": record.data, + "data": enriched_data, "version": "3.0.2", # Unity MCP version - "platform": os.name + "platform": _platform, + "source": _source, } - + if record.milestone: payload["milestone"] = record.milestone.value - - if not httpx: - return - - with httpx.Client(timeout=self.config.timeout) as client: - response = client.post(self.config.endpoint, json=payload) - - if response.status_code == 200: - logger.debug(f"Telemetry sent: {record.record_type}") - else: - logger.debug(f"Telemetry failed: HTTP {response.status_code}") - + + # Prefer httpx when available; otherwise fall back to urllib + if httpx: + with httpx.Client(timeout=self.config.timeout) as client: + response = client.post(self.config.endpoint, json=payload) + if response.status_code == 200: + logger.debug(f"Telemetry sent: {record.record_type}") + else: + logger.debug(f"Telemetry failed: HTTP {response.status_code}") + else: + import urllib.request + import urllib.error + data_bytes = json.dumps(payload).encode("utf-8") + req = urllib.request.Request( + self.config.endpoint, + data=data_bytes, + headers={"Content-Type": "application/json"}, + method="POST", + ) + try: + with urllib.request.urlopen(req, timeout=self.config.timeout) as resp: + if 200 <= resp.getcode() < 300: + logger.debug(f"Telemetry sent (urllib): {record.record_type}") + else: + logger.debug(f"Telemetry failed (urllib): HTTP {resp.getcode()}") + except urllib.error.URLError as ue: + logger.debug(f"Telemetry send failed (urllib): {ue}") + except Exception as e: # Never let telemetry errors interfere with app functionality logger.debug(f"Telemetry send failed: {e}") + # Global telemetry instance _telemetry_collector: Optional[TelemetryCollector] = None diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py index 8e1347a3d..875c66a41 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py @@ -4,39 +4,66 @@ import functools import time +import inspect +import logging from typing import Callable, Any from telemetry import record_tool_usage, record_milestone, MilestoneType +_log = logging.getLogger("unity-mcp-telemetry") +_decorator_log_count = 0 + def telemetry_tool(tool_name: str): """Decorator to add telemetry tracking to MCP tools""" def decorator(func: Callable) -> Callable: @functools.wraps(func) - def wrapper(*args, **kwargs) -> Any: + def _sync_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None - try: + global _decorator_log_count + if _decorator_log_count < 10: + _log.info(f"telemetry_decorator sync: tool={tool_name}") + _decorator_log_count += 1 result = func(*args, **kwargs) success = True - - # Record tool-specific milestones if tool_name == "manage_script" and kwargs.get("action") == "create": record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) elif tool_name.startswith("manage_scene"): record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - - # Record general first tool usage record_milestone(MilestoneType.FIRST_TOOL_USAGE) - return result - except Exception as e: error = str(e) raise finally: duration_ms = (time.time() - start_time) * 1000 record_tool_usage(tool_name, success, duration_ms, error) - - return wrapper + + @functools.wraps(func) + async def _async_wrapper(*args, **kwargs) -> Any: + start_time = time.time() + success = False + error = None + try: + global _decorator_log_count + if _decorator_log_count < 10: + _log.info(f"telemetry_decorator async: tool={tool_name}") + _decorator_log_count += 1 + result = await func(*args, **kwargs) + success = True + if tool_name == "manage_script" and kwargs.get("action") == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + return result + except Exception as e: + error = str(e) + raise + finally: + duration_ms = (time.time() - start_time) * 1000 + record_tool_usage(tool_name, success, duration_ms, error) + + return _async_wrapper if inspect.iscoroutinefunction(func) else _sync_wrapper return decorator \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py index a448465d5..27fa51a2d 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py @@ -7,12 +7,15 @@ from config import config import time +from telemetry_decorator import telemetry_tool + def register_execute_menu_item_tools(mcp: FastMCP): """Registers the execute_menu_item tool with the MCP server.""" @mcp.tool() + @telemetry_tool("execute_menu_item") async def execute_menu_item( - ctx: Context, + ctx: Any, menu_path: str, action: str = 'execute', parameters: Dict[str, Any] = None, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py index ccafb0477..ae50a8a32 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py @@ -9,12 +9,15 @@ from config import config import time +from telemetry_decorator import telemetry_tool + def register_manage_asset_tools(mcp: FastMCP): """Registers the manage_asset tool with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_asset") async def manage_asset( - ctx: Context, + ctx: Any, action: str, path: str, asset_type: str = None, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py index 8ff7378f2..f0edcec76 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py @@ -4,12 +4,16 @@ from unity_connection import get_unity_connection, send_command_with_retry from config import config +from telemetry_decorator import telemetry_tool +from telemetry import is_telemetry_enabled, record_tool_usage + def register_manage_editor_tools(mcp: FastMCP): """Register all editor management tools with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_editor") def manage_editor( - ctx: Context, + ctx: Any, action: str, wait_for_completion: bool = None, # --- Parameters for specific actions --- @@ -28,6 +32,13 @@ def manage_editor( Dictionary with operation results ('success', 'message', 'data'). """ try: + # Diagnostics: quick telemetry checks + if action == "telemetry_status": + return {"success": True, "telemetry_enabled": is_telemetry_enabled()} + + if action == "telemetry_ping": + record_tool_usage("diagnostic_ping", True, 1.0, None) + return {"success": True, "message": "telemetry ping queued"} # Prepare parameters, removing None values params = { "action": action, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py index cbe29a31f..b96a8bb22 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py @@ -4,12 +4,15 @@ from config import config import time +from telemetry_decorator import telemetry_tool + def register_manage_gameobject_tools(mcp: FastMCP): """Register all GameObject management tools with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_gameobject") def manage_gameobject( - ctx: Context, + ctx: Any, action: str, target: str = None, # GameObject identifier by name or path search_method: str = None, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py index c2257ef49..9041b5096 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py @@ -4,12 +4,15 @@ from config import config import time +from telemetry_decorator import telemetry_tool + def register_manage_scene_tools(mcp: FastMCP): """Register all scene management tools with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_scene") def manage_scene( - ctx: Context, + ctx: Any, action: str, name: str, path: str, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index 841ed200a..2602c2991 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -5,16 +5,8 @@ import os from urllib.parse import urlparse, unquote -try: - from telemetry_decorator import telemetry_tool - from telemetry import record_milestone, MilestoneType - HAS_TELEMETRY = True -except ImportError: - HAS_TELEMETRY = False - def telemetry_tool(tool_name: str): - def decorator(func): - return func - return decorator +from telemetry_decorator import telemetry_tool +from telemetry import record_milestone, MilestoneType def register_manage_script_tools(mcp: FastMCP): """Register all script management tools with the MCP server.""" @@ -92,7 +84,7 @@ def _split_uri(uri: str) -> tuple[str, str]: )) @telemetry_tool("apply_text_edits") def apply_text_edits( - ctx: Context, + ctx: Any, uri: str, edits: List[Dict[str, Any]], precondition_sha256: str | None = None, @@ -359,7 +351,7 @@ def _flip_async(): )) @telemetry_tool("create_script") def create_script( - ctx: Context, + ctx: Any, path: str, contents: str = "", script_type: str | None = None, @@ -397,7 +389,8 @@ def create_script( "Args: uri (unity://path/... or file://... or Assets/...).\n" "Rules: Target must resolve under Assets/.\n" )) - def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: + @telemetry_tool("delete_script") + def delete_script(ctx: Any, uri: str) -> Dict[str, Any]: """Delete a C# script by URI.""" name, directory = _split_uri(uri) if not directory or directory.split("/")[0].lower() != "assets": @@ -412,8 +405,9 @@ def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: "- basic: quick syntax checks.\n" "- standard: deeper checks (performance hints, common pitfalls).\n" )) + @telemetry_tool("validate_script") def validate_script( - ctx: Context, uri: str, level: str = "basic" + ctx: Any, uri: str, level: str = "basic" ) -> Dict[str, Any]: """Validate a C# script and return diagnostics.""" name, directory = _split_uri(uri) @@ -443,7 +437,7 @@ def validate_script( )) @telemetry_tool("manage_script") def manage_script( - ctx: Context, + ctx: Any, action: str, name: str, path: str, @@ -573,7 +567,8 @@ def manage_script( "Get manage_script capabilities (supported ops, limits, and guards).\n\n" "Returns:\n- ops: list of supported structured ops\n- text_ops: list of supported text ops\n- max_edit_payload_bytes: server edit payload cap\n- guards: header/using guard enabled flag\n" )) - def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: + @telemetry_tool("manage_script_capabilities") + def manage_script_capabilities(ctx: Any) -> Dict[str, Any]: try: # Keep in sync with server/Editor ManageScript implementation ops = [ @@ -600,7 +595,8 @@ def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: "Args: uri (unity://path/Assets/... or file://... or Assets/...).\n" "Returns: {sha256, lengthBytes, lastModifiedUtc, uri, path}." )) - def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: + @telemetry_tool("get_sha") + def get_sha(ctx: Any, uri: str) -> Dict[str, Any]: """Return SHA256 and basic metadata for a script.""" try: name, directory = _split_uri(uri) diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py index 91a107b9f..a6f800473 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py @@ -5,6 +5,8 @@ import os from unity_connection import send_command_with_retry +from telemetry_decorator import telemetry_tool + def _apply_edits_locally(original_text: str, edits: List[Dict[str, Any]]) -> str: text = original_text @@ -326,8 +328,9 @@ def register_manage_script_edits_tools(mcp: FastMCP): " 'position':'after','afterMethodName':'GetCurrentTarget' }\n" "] }\n" )) + @telemetry_tool("script_apply_edits") def script_apply_edits( - ctx: Context, + ctx: Any, name: str, path: str, edits: List[Dict[str, Any]], diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py index 8ddb6c7c4..abf1d7028 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_shader.py @@ -6,12 +6,15 @@ import os import base64 +from telemetry_decorator import telemetry_tool + def register_manage_shader_tools(mcp: FastMCP): """Register all shader script management tools with the MCP server.""" @mcp.tool() + @telemetry_tool("manage_shader") def manage_shader( - ctx: Context, + ctx: Any, action: str, name: str, path: str, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py index aae0e49f7..543684418 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py @@ -7,12 +7,15 @@ from unity_connection import get_unity_connection, send_command_with_retry from config import config +from telemetry_decorator import telemetry_tool + def register_read_console_tools(mcp: FastMCP): """Registers the read_console tool with the MCP server.""" @mcp.tool() + @telemetry_tool("read_console") def read_console( - ctx: Context, + ctx: Any, action: str = None, types: List[str] = None, count: int = None, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py index 909cb3c1a..38ea3f94c 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py @@ -5,7 +5,7 @@ """ from __future__ import annotations -from typing import Dict, Any, List +from typing import Dict, Any, List, Optional import re from pathlib import Path from urllib.parse import urlparse, unquote @@ -13,6 +13,8 @@ import hashlib import os +from telemetry_decorator import telemetry_tool + from mcp.server.fastmcp import FastMCP, Context from unity_connection import send_command_with_retry @@ -114,8 +116,9 @@ def register_resource_tools(mcp: FastMCP) -> None: "Security: restricted to Assets/ subtree; symlinks are resolved and must remain under Assets/.\n" "Notes: Only .cs files are returned by default; always appends unity://spec/script-edits.\n" )) + @telemetry_tool("list_resources") async def list_resources( - ctx: Context | None = None, + ctx: Any = None, pattern: str | None = "*.cs", under: str = "Assets", limit: int = 200, @@ -174,9 +177,10 @@ async def list_resources( "Security: uri must resolve under Assets/.\n" "Examples: head_bytes=1024; start_line=100,line_count=40; tail_lines=120.\n" )) + @telemetry_tool("read_resource") async def read_resource( uri: str, - ctx: Context | None = None, + ctx: Any = None, start_line: int | None = None, line_count: int | None = None, head_bytes: int | None = None, @@ -334,10 +338,11 @@ async def read_resource( return {"success": False, "error": str(e)} @mcp.tool() + @telemetry_tool("find_in_file") async def find_in_file( uri: str, pattern: str, - ctx: Context | None = None, + ctx: Any = None, ignore_case: bool | None = True, project_root: str | None = None, max_results: int | None = 1, From bbe4b07558645cb3ae8e5aa551f952c2fba88f72 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:06:04 -0700 Subject: [PATCH 04/21] feat: improve telemetry and parameter validation - Add server-side integer coercion for numeric parameters in all tools - Fix parameter type validation issues (read_resource, find_in_file, read_console, manage_scene, manage_asset) - Add proper tool descriptions with ctx parameter documentation - Fix Context type annotations (use Context instead of Any for ctx) - All tools now accept flexible numeric inputs (strings, floats) and coerce to integers - Telemetry system working with all tool_execution events captured in BigQuery - Remove invalid parameter type warnings from client-side validation --- .../UnityMcpServer~/src/tools/manage_asset.py | 23 ++- .../src/tools/manage_editor.py | 25 +-- .../UnityMcpServer~/src/tools/manage_scene.py | 20 ++- .../src/tools/manage_script.py | 41 +++-- .../src/tools/manage_script_edits.py | 41 +++-- .../UnityMcpServer~/src/tools/read_console.py | 57 +++---- .../src/tools/resource_tools.py | 143 +++++++++--------- 7 files changed, 198 insertions(+), 152 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py index ae50a8a32..49a8aeeea 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_asset.py @@ -27,8 +27,8 @@ async def manage_asset( search_pattern: str = None, filter_type: str = None, filter_date_after: str = None, - page_size: int = None, - page_number: int = None + page_size: Any = None, + page_number: Any = None ) -> Dict[str, Any]: """Performs asset operations (import, create, modify, delete, etc.) in Unity. @@ -53,6 +53,25 @@ async def manage_asset( if properties is None: properties = {} + # Coerce numeric inputs defensively + def _coerce_int(value, default=None): + if value is None: + return default + try: + if isinstance(value, bool): + return default + if isinstance(value, int): + return int(value) + s = str(value).strip() + if s.lower() in ("", "none", "null"): + return default + return int(float(s)) + except Exception: + return default + + page_size = _coerce_int(page_size) + page_number = _coerce_int(page_number) + # Prepare parameters for the C# handler params_dict = { "action": action.lower(), diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py index f0edcec76..f5508a4e6 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_editor.py @@ -10,10 +10,21 @@ def register_manage_editor_tools(mcp: FastMCP): """Register all editor management tools with the MCP server.""" - @mcp.tool() + @mcp.tool(description=( + "Controls and queries the Unity editor's state and settings.\n\n" + "Args:\n" + "- ctx: Context object (required)\n" + "- action: Operation (e.g., 'play', 'pause', 'get_state', 'set_active_tool', 'add_tag')\n" + "- wait_for_completion: Optional. If True, waits for certain actions\n" + "- tool_name: Tool name for specific actions\n" + "- tag_name: Tag name for specific actions\n" + "- layer_name: Layer name for specific actions\n\n" + "Returns:\n" + "Dictionary with operation results ('success', 'message', 'data')." + )) @telemetry_tool("manage_editor") def manage_editor( - ctx: Any, + ctx: Context, action: str, wait_for_completion: bool = None, # --- Parameters for specific actions --- @@ -21,16 +32,6 @@ def manage_editor( tag_name: str = None, layer_name: str = None, ) -> Dict[str, Any]: - """Controls and queries the Unity editor's state and settings. - - Args: - action: Operation (e.g., 'play', 'pause', 'get_state', 'set_active_tool', 'add_tag'). - wait_for_completion: Optional. If True, waits for certain actions. - Action-specific arguments (e.g., tool_name, tag_name, layer_name). - - Returns: - Dictionary with operation results ('success', 'message', 'data'). - """ try: # Diagnostics: quick telemetry checks if action == "telemetry_status": diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py index 9041b5096..ef9272528 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py @@ -16,7 +16,7 @@ def manage_scene( action: str, name: str, path: str, - build_index: int, + build_index: Any, ) -> Dict[str, Any]: """Manages Unity scenes (load, save, create, get hierarchy, etc.). @@ -31,6 +31,24 @@ def manage_scene( Dictionary with results ('success', 'message', 'data'). """ try: + # Coerce numeric inputs defensively + def _coerce_int(value, default=None): + if value is None: + return default + try: + if isinstance(value, bool): + return default + if isinstance(value, int): + return int(value) + s = str(value).strip() + if s.lower() in ("", "none", "null"): + return default + return int(float(s)) + except Exception: + return default + + build_index = _coerce_int(build_index, default=0) + params = { "action": action, "name": name, diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index 2602c2991..f702801ef 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -5,8 +5,16 @@ import os from urllib.parse import urlparse, unquote -from telemetry_decorator import telemetry_tool -from telemetry import record_milestone, MilestoneType +try: + from telemetry_decorator import telemetry_tool + from telemetry import record_milestone, MilestoneType + HAS_TELEMETRY = True +except ImportError: + HAS_TELEMETRY = False + def telemetry_tool(tool_name: str): + def decorator(func): + return func + return decorator def register_manage_script_tools(mcp: FastMCP): """Register all script management tools with the MCP server.""" @@ -84,7 +92,7 @@ def _split_uri(uri: str) -> tuple[str, str]: )) @telemetry_tool("apply_text_edits") def apply_text_edits( - ctx: Any, + ctx: Context, uri: str, edits: List[Dict[str, Any]], precondition_sha256: str | None = None, @@ -351,7 +359,7 @@ def _flip_async(): )) @telemetry_tool("create_script") def create_script( - ctx: Any, + ctx: Context, path: str, contents: str = "", script_type: str | None = None, @@ -390,7 +398,7 @@ def create_script( "Rules: Target must resolve under Assets/.\n" )) @telemetry_tool("delete_script") - def delete_script(ctx: Any, uri: str) -> Dict[str, Any]: + def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: """Delete a C# script by URI.""" name, directory = _split_uri(uri) if not directory or directory.split("/")[0].lower() != "assets": @@ -407,7 +415,7 @@ def delete_script(ctx: Any, uri: str) -> Dict[str, Any]: )) @telemetry_tool("validate_script") def validate_script( - ctx: Any, uri: str, level: str = "basic" + ctx: Context, uri: str, level: str = "basic" ) -> Dict[str, Any]: """Validate a C# script and return diagnostics.""" name, directory = _split_uri(uri) @@ -422,11 +430,6 @@ def validate_script( "level": level, } resp = send_command_with_retry("manage_script", params) - if isinstance(resp, dict) and resp.get("success"): - diags = resp.get("data", {}).get("diagnostics", []) or [] - warnings = sum(d.get("severity", "").lower() == "warning" for d in diags) - errors = sum(d.get("severity", "").lower() in ("error", "fatal") for d in diags) - return {"success": True, "data": {"warnings": warnings, "errors": errors}} return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} @mcp.tool(description=( @@ -437,7 +440,7 @@ def validate_script( )) @telemetry_tool("manage_script") def manage_script( - ctx: Any, + ctx: Context, action: str, name: str, path: str, @@ -565,10 +568,11 @@ def manage_script( @mcp.tool(description=( "Get manage_script capabilities (supported ops, limits, and guards).\n\n" + "Args:\n- random_string: required parameter (any string value)\n\n" "Returns:\n- ops: list of supported structured ops\n- text_ops: list of supported text ops\n- max_edit_payload_bytes: server edit payload cap\n- guards: header/using guard enabled flag\n" )) @telemetry_tool("manage_script_capabilities") - def manage_script_capabilities(ctx: Any) -> Dict[str, Any]: + def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: try: # Keep in sync with server/Editor ManageScript implementation ops = [ @@ -596,21 +600,12 @@ def manage_script_capabilities(ctx: Any) -> Dict[str, Any]: "Returns: {sha256, lengthBytes, lastModifiedUtc, uri, path}." )) @telemetry_tool("get_sha") - def get_sha(ctx: Any, uri: str) -> Dict[str, Any]: + def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: """Return SHA256 and basic metadata for a script.""" try: name, directory = _split_uri(uri) params = {"action": "get_sha", "name": name, "path": directory} resp = send_command_with_retry("manage_script", params) - if isinstance(resp, dict) and resp.get("success"): - data = resp.get("data", {}) - return { - "success": True, - "data": { - "sha256": data.get("sha256"), - "lengthBytes": data.get("lengthBytes"), - }, - } return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} except Exception as e: return {"success": False, "message": f"get_sha error: {e}"} diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py index a6f800473..3d66da007 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script_edits.py @@ -1,5 +1,5 @@ from mcp.server.fastmcp import FastMCP, Context -from typing import Dict, Any, List, Tuple +from typing import Dict, Any, List, Tuple, Optional import base64 import re import os @@ -318,23 +318,42 @@ def register_manage_script_edits_tools(mcp: FastMCP): "Do NOT use: new_method, anchor_method, content, newText (aliases accepted but normalized).\n\n" "Examples:\n" "1) Replace a method:\n" - "{ 'name':'SmartReach','path':'Assets/Scripts/Interaction','edits':[\n" - " { 'op':'replace_method','className':'SmartReach','methodName':'HasTarget',\n" - " 'replacement':'public bool HasTarget(){ return currentTarget!=null; }' }\n" - "], 'options':{'validate':'standard','refresh':'immediate'} }\n\n" + "{\n" + " \"name\": \"SmartReach\",\n" + " \"path\": \"Assets/Scripts/Interaction\",\n" + " \"edits\": [\n" + " {\n" + " \"op\": \"replace_method\",\n" + " \"className\": \"SmartReach\",\n" + " \"methodName\": \"HasTarget\",\n" + " \"replacement\": \"public bool HasTarget(){ return currentTarget!=null; }\"\n" + " }\n" + " ],\n" + " \"options\": {\"validate\": \"standard\", \"refresh\": \"immediate\"}\n" + "}\n\n" "2) Insert a method after another:\n" - "{ 'name':'SmartReach','path':'Assets/Scripts/Interaction','edits':[\n" - " { 'op':'insert_method','className':'SmartReach','replacement':'public void PrintSeries(){ Debug.Log(seriesName); }',\n" - " 'position':'after','afterMethodName':'GetCurrentTarget' }\n" - "] }\n" + "{\n" + " \"name\": \"SmartReach\",\n" + " \"path\": \"Assets/Scripts/Interaction\",\n" + " \"edits\": [\n" + " {\n" + " \"op\": \"insert_method\",\n" + " \"className\": \"SmartReach\",\n" + " \"replacement\": \"public void PrintSeries(){ Debug.Log(seriesName); }\",\n" + " \"position\": \"after\",\n" + " \"afterMethodName\": \"GetCurrentTarget\"\n" + " }\n" + " ]\n" + "}\n\n" + "Note: 'options' must be an object/dict, not a string. Use proper JSON syntax.\n" )) @telemetry_tool("script_apply_edits") def script_apply_edits( - ctx: Any, + ctx: Context, name: str, path: str, edits: List[Dict[str, Any]], - options: Dict[str, Any] | None = None, + options: Optional[Dict[str, Any]] = None, script_type: str = "MonoBehaviour", namespace: str = "", ) -> Dict[str, Any]: diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py index 543684418..36fcce4a6 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py @@ -6,7 +6,6 @@ from mcp.server.fastmcp import FastMCP, Context from unity_connection import get_unity_connection, send_command_with_retry from config import config - from telemetry_decorator import telemetry_tool def register_read_console_tools(mcp: FastMCP): @@ -15,10 +14,10 @@ def register_read_console_tools(mcp: FastMCP): @mcp.tool() @telemetry_tool("read_console") def read_console( - ctx: Any, + ctx: Context, action: str = None, types: List[str] = None, - count: int = None, + count: Any = None, filter_text: str = None, since_timestamp: str = None, format: str = None, @@ -43,21 +42,34 @@ def read_console( # Get the connection instance bridge = get_unity_connection() - # Set defaults if values are None (conservative but useful for CI) + # Set defaults if values are None action = action if action is not None else 'get' - types = types if types is not None else ['error'] - # Normalize types if passed as a single string - if isinstance(types, str): - types = [types] - format = format if format is not None else 'json' + types = types if types is not None else ['error', 'warning', 'log'] + format = format if format is not None else 'detailed' include_stacktrace = include_stacktrace if include_stacktrace is not None else True - # Default count to a higher value unless explicitly provided - count = 50 if count is None else count # Normalize action if it's a string if isinstance(action, str): action = action.lower() + # Coerce count defensively (string/float -> int) + def _coerce_int(value, default=None): + if value is None: + return default + try: + if isinstance(value, bool): + return default + if isinstance(value, int): + return int(value) + s = str(value).strip() + if s.lower() in ("", "none", "null"): + return default + return int(float(s)) + except Exception: + return default + + count = _coerce_int(count) + # Prepare parameters for the C# handler params_dict = { "action": action, @@ -76,25 +88,6 @@ def read_console( if 'count' not in params_dict: params_dict['count'] = None - # Use centralized retry helper (tolerate legacy list payloads from some agents) + # Use centralized retry helper resp = send_command_with_retry("read_console", params_dict) - if isinstance(resp, dict) and resp.get("success") and not include_stacktrace: - data = resp.get("data", {}) or {} - lines = data.get("lines") - if lines is None: - # Some handlers return the raw list under data - lines = data if isinstance(data, list) else [] - - def _entry(x: Any) -> Dict[str, Any]: - if isinstance(x, dict): - return { - "level": x.get("level") or x.get("type"), - "message": x.get("message") or x.get("text"), - } - if isinstance(x, (list, tuple)) and len(x) >= 2: - return {"level": x[0], "message": x[1]} - return {"level": None, "message": str(x)} - - trimmed = [_entry(l) for l in (lines or [])] - return {"success": True, "data": {"lines": trimmed}} - return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} + return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py index 38ea3f94c..a7b4dd2c9 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py @@ -3,7 +3,6 @@ can still list and read files via normal tools. These call into the same safe path logic (re-implemented here to avoid importing server.py). """ -from __future__ import annotations from typing import Dict, Any, List, Optional import re @@ -13,12 +12,35 @@ import hashlib import os -from telemetry_decorator import telemetry_tool - from mcp.server.fastmcp import FastMCP, Context +from telemetry_decorator import telemetry_tool from unity_connection import send_command_with_retry +def _coerce_int(value: Any, default: Optional[int] = None, minimum: Optional[int] = None) -> Optional[int]: + """Safely coerce various inputs (str/float/etc.) to an int. + Returns default on failure; clamps to minimum when provided. + """ + if value is None: + return default + try: + # Avoid treating booleans as ints implicitly + if isinstance(value, bool): + return default + if isinstance(value, int): + result = int(value) + else: + s = str(value).strip() + if s.lower() in ("", "none", "null"): + return default + # Allow "10.0" or similar inputs + result = int(float(s)) + if minimum is not None and result < minimum: + return minimum + return result + except Exception: + return default + def _resolve_project_root(override: str | None) -> Path: # 1) Explicit override if override: @@ -118,11 +140,11 @@ def register_resource_tools(mcp: FastMCP) -> None: )) @telemetry_tool("list_resources") async def list_resources( - ctx: Any = None, - pattern: str | None = "*.cs", + ctx: Optional[Context] = None, + pattern: Optional[str] = "*.cs", under: str = "Assets", - limit: int = 200, - project_root: str | None = None, + limit: Any = 200, + project_root: Optional[str] = None, ) -> Dict[str, Any]: """ Lists project URIs (unity://path/...) under a folder (default: Assets). @@ -144,6 +166,7 @@ async def list_resources( return {"success": False, "error": "Listing is restricted to Assets/"} matches: List[str] = [] + limit_int = _coerce_int(limit, default=200, minimum=1) for p in base.rglob("*"): if not p.is_file(): continue @@ -160,7 +183,7 @@ async def list_resources( continue rel = p.relative_to(project).as_posix() matches.append(f"unity://path/{rel}") - if len(matches) >= max(1, limit): + if len(matches) >= max(1, limit_int): break # Always include the canonical spec resource so NL clients can discover it @@ -180,19 +203,17 @@ async def list_resources( @telemetry_tool("read_resource") async def read_resource( uri: str, - ctx: Any = None, - start_line: int | None = None, - line_count: int | None = None, - head_bytes: int | None = None, - tail_lines: int | None = None, - project_root: str | None = None, - request: str | None = None, - include_text: bool = False, + ctx: Optional[Context] = None, + start_line: Any = None, + line_count: Any = None, + head_bytes: Any = None, + tail_lines: Any = None, + project_root: Optional[str] = None, + request: Optional[str] = None, ) -> Dict[str, Any]: """ Reads a resource by unity://path/... URI with optional slicing. - By default only the SHA-256 hash and byte length are returned; set - ``include_text`` or provide window arguments to receive text. + One of line window (start_line/line_count) or head_bytes can be used to limit size. """ try: # Serve the canonical spec directly when requested (allow bare or with scheme) @@ -297,43 +318,31 @@ async def read_resource( start_line = max(1, hit_line - half) line_count = window - raw = p.read_bytes() - sha = hashlib.sha256(raw).hexdigest() - length = len(raw) + # Coerce numeric inputs defensively (string/float -> int) + start_line = _coerce_int(start_line) + line_count = _coerce_int(line_count) + head_bytes = _coerce_int(head_bytes, minimum=1) + tail_lines = _coerce_int(tail_lines, minimum=1) - want_text = ( - bool(include_text) - or (head_bytes is not None and head_bytes >= 0) - or (tail_lines is not None and tail_lines > 0) - or (start_line is not None and line_count is not None) - ) - if want_text: - text: str - if head_bytes is not None and head_bytes >= 0: - text = raw[: head_bytes].decode("utf-8", errors="replace") - else: - text = raw.decode("utf-8", errors="replace") - if tail_lines is not None and tail_lines > 0: - lines = text.splitlines() - n = max(0, tail_lines) - text = "\n".join(lines[-n:]) - elif ( - start_line is not None - and line_count is not None - and line_count >= 0 - ): - lines = text.splitlines() - s = max(0, start_line - 1) - e = min(len(lines), s + line_count) - text = "\n".join(lines[s:e]) - return { - "success": True, - "data": {"text": text, "metadata": {"sha256": sha}}, - } - return { - "success": True, - "data": {"metadata": {"sha256": sha, "lengthBytes": length}}, - } + # Mutually exclusive windowing options precedence: + # 1) head_bytes, 2) tail_lines, 3) start_line+line_count, else full text + if head_bytes and head_bytes > 0: + raw = p.read_bytes()[: head_bytes] + text = raw.decode("utf-8", errors="replace") + else: + text = p.read_text(encoding="utf-8") + if tail_lines is not None and tail_lines > 0: + lines = text.splitlines() + n = max(0, tail_lines) + text = "\n".join(lines[-n:]) + elif start_line is not None and line_count is not None and line_count >= 0: + lines = text.splitlines() + s = max(0, start_line - 1) + e = min(len(lines), s + line_count) + text = "\n".join(lines[s:e]) + + sha = hashlib.sha256(text.encode("utf-8")).hexdigest() + return {"success": True, "data": {"text": text, "metadata": {"sha256": sha}}} except Exception as e: return {"success": False, "error": str(e)} @@ -342,13 +351,13 @@ async def read_resource( async def find_in_file( uri: str, pattern: str, - ctx: Any = None, - ignore_case: bool | None = True, - project_root: str | None = None, - max_results: int | None = 1, + ctx: Optional[Context] = None, + ignore_case: Optional[bool] = True, + project_root: Optional[str] = None, + max_results: Any = 200, ) -> Dict[str, Any]: """ - Searches a file with a regex pattern and returns match positions only. + Searches a file with a regex pattern and returns line numbers and excerpts. - uri: unity://path/Assets/... or file path form supported by read_resource - pattern: regular expression (Python re) - ignore_case: case-insensitive by default @@ -368,20 +377,12 @@ async def find_in_file( rx = re.compile(pattern, flags) results = [] + max_results_int = _coerce_int(max_results, default=200, minimum=1) lines = text.splitlines() for i, line in enumerate(lines, start=1): - m = rx.search(line) - if m: - start_col, end_col = m.span() - results.append( - { - "startLine": i, - "startCol": start_col + 1, - "endLine": i, - "endCol": end_col + 1, - } - ) - if max_results and len(results) >= max_results: + if rx.search(line): + results.append({"line": i, "text": line}) + if max_results_int and len(results) >= max_results_int: break return {"success": True, "data": {"matches": results, "count": len(results)}} From 979757e38a22136f1c4f955f6959bd6cdd36f5b7 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:14:49 -0700 Subject: [PATCH 05/21] tests green: align SDK outputs + harden inputs\n\n- find_in_file: start/end positions with 1-based exclusive endCol\n- read_resource: metadata-only default + lengthBytes; selection returns text\n- read_console: strip stacktrace when include_stacktrace=false\n- validate_script: summary counts; get_sha: minimal fields\n- silence stdout in test_telemetry helper --- .../UnityMcpServer~/src/test_telemetry.py | 62 +++++++++---------- .../src/tools/manage_script.py | 9 +++ .../UnityMcpServer~/src/tools/read_console.py | 11 +++- .../src/tools/resource_tools.py | 56 +++++++++++------ 4 files changed, 84 insertions(+), 54 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py index 850f3b912..c9e3013a7 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/test_telemetry.py @@ -14,20 +14,20 @@ def test_telemetry_basic(): """Test basic telemetry functionality""" - print("๐Ÿงช Testing Unity MCP Telemetry System...") + # Avoid stdout noise in tests try: from telemetry import ( get_telemetry, record_telemetry, record_milestone, RecordType, MilestoneType, is_telemetry_enabled ) - print("โœ… Telemetry module imported successfully") + pass except ImportError as e: - print(f"โŒ Failed to import telemetry module: {e}") + # Silent failure path for tests return False # Test telemetry enabled status - print(f"๐Ÿ“Š Telemetry enabled: {is_telemetry_enabled()}") + _ = is_telemetry_enabled() # Test basic record try: @@ -35,9 +35,9 @@ def test_telemetry_basic(): "version": "3.0.2", "test_run": True }) - print("โœ… Basic telemetry record sent") + pass except Exception as e: - print(f"โŒ Failed to send basic telemetry: {e}") + # Silent failure path for tests return False # Test milestone recording @@ -45,24 +45,24 @@ def test_telemetry_basic(): is_first = record_milestone(MilestoneType.FIRST_STARTUP, { "test_mode": True }) - print(f"โœ… Milestone recorded (first time: {is_first})") + _ = is_first except Exception as e: - print(f"โŒ Failed to record milestone: {e}") + # Silent failure path for tests return False # Test telemetry collector try: collector = get_telemetry() - print(f"โœ… Telemetry collector initialized (UUID: {collector._customer_uuid[:8]}...)") + _ = collector except Exception as e: - print(f"โŒ Failed to get telemetry collector: {e}") + # Silent failure path for tests return False return True def test_telemetry_disabled(): """Test telemetry with disabled state""" - print("\n๐Ÿšซ Testing telemetry disabled state...") + # Silent for tests # Set environment variable to disable telemetry os.environ["DISABLE_TELEMETRY"] = "true" @@ -74,23 +74,23 @@ def test_telemetry_disabled(): from telemetry import is_telemetry_enabled, record_telemetry, RecordType - print(f"๐Ÿ“Š Telemetry enabled (should be False): {is_telemetry_enabled()}") + _ = is_telemetry_enabled() if not is_telemetry_enabled(): - print("โœ… Telemetry correctly disabled via environment variable") + pass # Test that records are ignored when disabled record_telemetry(RecordType.USAGE, {"test": "should_be_ignored"}) - print("โœ… Telemetry record ignored when disabled") + pass return True else: - print("โŒ Telemetry not disabled by environment variable") + pass return False def test_data_storage(): """Test data storage functionality""" - print("\n๐Ÿ’พ Testing data storage...") + # Silent for tests try: from telemetry import get_telemetry @@ -98,31 +98,28 @@ def test_data_storage(): collector = get_telemetry() data_dir = collector.config.data_dir - print(f"๐Ÿ“ Data directory: {data_dir}") - print(f"๐Ÿท๏ธ UUID file: {collector.config.uuid_file}") - print(f"๐ŸŽฏ Milestones file: {collector.config.milestones_file}") + _ = (data_dir, collector.config.uuid_file, collector.config.milestones_file) # Check if files exist if collector.config.uuid_file.exists(): - print("โœ… UUID file exists") + pass else: - print("โ„น๏ธ UUID file will be created on first use") + pass if collector.config.milestones_file.exists(): - print("โœ… Milestones file exists") + pass else: - print("โ„น๏ธ Milestones file will be created on first milestone") + pass return True except Exception as e: - print(f"โŒ Data storage test failed: {e}") + # Silent failure path for tests return False def main(): """Run all telemetry tests""" - print("๐Ÿš€ Unity MCP Telemetry Test Suite") - print("=" * 50) + # Silent runner for CI tests = [ test_telemetry_basic, @@ -137,22 +134,21 @@ def main(): try: if test(): passed += 1 - print("โœ… PASSED\n") + pass else: failed += 1 - print("โŒ FAILED\n") + pass except Exception as e: failed += 1 - print(f"โŒ FAILED with exception: {e}\n") + pass - print("=" * 50) - print(f"๐Ÿ“Š Test Results: {passed} passed, {failed} failed") + _ = (passed, failed) if failed == 0: - print("๐ŸŽ‰ All telemetry tests passed!") + pass return True else: - print(f"โš ๏ธ {failed} test(s) failed") + pass return False if __name__ == "__main__": diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index f702801ef..149f06efe 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -430,6 +430,11 @@ def validate_script( "level": level, } resp = send_command_with_retry("manage_script", params) + if isinstance(resp, dict) and resp.get("success"): + diags = resp.get("data", {}).get("diagnostics", []) + warnings = sum(1 for d in diags if str(d.get("severity", "")).lower() in ("warning",)) + errors = sum(1 for d in diags if str(d.get("severity", "")).lower() in ("error", "fatal")) + return {"success": True, "data": {"warnings": warnings, "errors": errors}} return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} @mcp.tool(description=( @@ -606,6 +611,10 @@ def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: name, directory = _split_uri(uri) params = {"action": "get_sha", "name": name, "path": directory} resp = send_command_with_retry("manage_script", params) + if isinstance(resp, dict) and resp.get("success"): + data = resp.get("data", {}) + minimal = {"sha256": data.get("sha256"), "lengthBytes": data.get("lengthBytes")} + return {"success": True, "data": minimal} return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} except Exception as e: return {"success": False, "message": f"get_sha error: {e}"} diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py index 36fcce4a6..9b3984519 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/read_console.py @@ -90,4 +90,13 @@ def _coerce_int(value, default=None): # Use centralized retry helper resp = send_command_with_retry("read_console", params_dict) - return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} \ No newline at end of file + if isinstance(resp, dict) and resp.get("success") and not include_stacktrace: + # Strip stacktrace fields from returned lines if present + try: + lines = resp.get("data", {}).get("lines", []) + for line in lines: + if isinstance(line, dict) and "stacktrace" in line: + line.pop("stacktrace", None) + except Exception: + pass + return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py index a7b4dd2c9..5024fd4a8 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/resource_tools.py @@ -324,25 +324,33 @@ async def read_resource( head_bytes = _coerce_int(head_bytes, minimum=1) tail_lines = _coerce_int(tail_lines, minimum=1) - # Mutually exclusive windowing options precedence: - # 1) head_bytes, 2) tail_lines, 3) start_line+line_count, else full text - if head_bytes and head_bytes > 0: - raw = p.read_bytes()[: head_bytes] - text = raw.decode("utf-8", errors="replace") - else: - text = p.read_text(encoding="utf-8") - if tail_lines is not None and tail_lines > 0: - lines = text.splitlines() - n = max(0, tail_lines) - text = "\n".join(lines[-n:]) - elif start_line is not None and line_count is not None and line_count >= 0: - lines = text.splitlines() - s = max(0, start_line - 1) - e = min(len(lines), s + line_count) - text = "\n".join(lines[s:e]) + # Compute SHA over full file contents (metadata-only default) + full_bytes = p.read_bytes() + full_sha = hashlib.sha256(full_bytes).hexdigest() - sha = hashlib.sha256(text.encode("utf-8")).hexdigest() - return {"success": True, "data": {"text": text, "metadata": {"sha256": sha}}} + # Selection only when explicitly requested via windowing args or request text hints + selection_requested = bool(head_bytes or tail_lines or (start_line is not None and line_count is not None) or request) + if selection_requested: + # Mutually exclusive windowing options precedence: + # 1) head_bytes, 2) tail_lines, 3) start_line+line_count, else full text + if head_bytes and head_bytes > 0: + raw = full_bytes[: head_bytes] + text = raw.decode("utf-8", errors="replace") + else: + text = full_bytes.decode("utf-8", errors="replace") + if tail_lines is not None and tail_lines > 0: + lines = text.splitlines() + n = max(0, tail_lines) + text = "\n".join(lines[-n:]) + elif start_line is not None and line_count is not None and line_count >= 0: + lines = text.splitlines() + s = max(0, start_line - 1) + e = min(len(lines), s + line_count) + text = "\n".join(lines[s:e]) + return {"success": True, "data": {"text": text, "metadata": {"sha256": full_sha, "lengthBytes": len(full_bytes)}}} + else: + # Default: metadata only + return {"success": True, "data": {"metadata": {"sha256": full_sha, "lengthBytes": len(full_bytes)}}} except Exception as e: return {"success": False, "error": str(e)} @@ -380,8 +388,16 @@ async def find_in_file( max_results_int = _coerce_int(max_results, default=200, minimum=1) lines = text.splitlines() for i, line in enumerate(lines, start=1): - if rx.search(line): - results.append({"line": i, "text": line}) + m = rx.search(line) + if m: + start_col = m.start() + 1 # 1-based + end_col = m.end() + 1 # 1-based, end exclusive + results.append({ + "startLine": i, + "startCol": start_col, + "endLine": i, + "endCol": end_col, + }) if max_results_int and len(results) >= max_results_int: break From 7f0527f708fcb0ceaf560f80805f48f3f5d84c9c Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:25:07 -0700 Subject: [PATCH 06/21] chore: apply CodeRabbit suggestions - README path separators (todo in separate doc commit) - manage_gameobject: pop prefabFolder not prefab_folder - execute_menu_item: make sync to avoid blocking event loop - telemetry: validate endpoint scheme (allow http/https only) and re-validate at send time --- .../UnityMcpServer~/src/telemetry.py | 35 ++++++++++++++++--- .../src/tools/execute_menu_item.py | 2 +- .../src/tools/manage_gameobject.py | 4 +-- 3 files changed, 33 insertions(+), 8 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index b701e2a57..3141d1ba5 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -13,6 +13,7 @@ import platform import logging from enum import Enum +from urllib.parse import urlparse from dataclasses import dataclass, asdict from typing import Optional, Dict, Any, List from pathlib import Path @@ -64,9 +65,11 @@ def __init__(self): self.enabled = not self._is_disabled() # Telemetry endpoint (Cloud Run default; override via env) - self.endpoint = os.environ.get( - "UNITY_MCP_TELEMETRY_ENDPOINT", - "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" + default_ep = "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" + self.default_endpoint = default_ep + self.endpoint = self._validated_endpoint( + os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep), + default_ep, ) # Local storage for UUID and milestones @@ -109,6 +112,25 @@ def _get_data_directory(self) -> Path: data_dir.mkdir(parents=True, exist_ok=True) return data_dir + def _validated_endpoint(self, candidate: str, fallback: str) -> str: + """Validate telemetry endpoint URL scheme; allow only http/https. + Falls back to the provided default on error. + """ + try: + parsed = urlparse(candidate) + if parsed.scheme not in ("https", "http"): + raise ValueError(f"Unsupported scheme: {parsed.scheme}") + # Basic sanity: require network location and path + if not parsed.netloc: + raise ValueError("Missing netloc in endpoint") + return candidate + except Exception as e: + logger.debug( + f"Invalid telemetry endpoint '{candidate}', using default. Error: {e}", + exc_info=True, + ) + return fallback + class TelemetryCollector: """Main telemetry collection class""" @@ -227,7 +249,9 @@ def _send_telemetry(self, record: TelemetryRecord): # Prefer httpx when available; otherwise fall back to urllib if httpx: with httpx.Client(timeout=self.config.timeout) as client: - response = client.post(self.config.endpoint, json=payload) + # Re-validate endpoint at send time to handle dynamic changes + endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint) + response = client.post(endpoint, json=payload) if response.status_code == 200: logger.debug(f"Telemetry sent: {record.record_type}") else: @@ -236,8 +260,9 @@ def _send_telemetry(self, record: TelemetryRecord): import urllib.request import urllib.error data_bytes = json.dumps(payload).encode("utf-8") + endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint) req = urllib.request.Request( - self.config.endpoint, + endpoint, data=data_bytes, headers={"Content-Type": "application/json"}, method="POST", diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py index 27fa51a2d..c21ecb89d 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/execute_menu_item.py @@ -14,7 +14,7 @@ def register_execute_menu_item_tools(mcp: FastMCP): @mcp.tool() @telemetry_tool("execute_menu_item") - async def execute_menu_item( + def execute_menu_item( ctx: Any, menu_path: str, action: str = 'execute', diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py index b96a8bb22..a2ffe0ea0 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_gameobject.py @@ -122,9 +122,9 @@ def manage_gameobject( params["prefabPath"] = constructed_path.replace("\\", "/") elif not params["prefabPath"].lower().endswith(".prefab"): return {"success": False, "message": f"Invalid prefab_path: '{params['prefabPath']}' must end with .prefab"} - # Ensure prefab_folder itself isn't sent if prefabPath was constructed or provided + # Ensure prefabFolder itself isn't sent if prefabPath was constructed or provided # The C# side only needs the final prefabPath - params.pop("prefab_folder", None) + params.pop("prefabFolder", None) # -------------------------------- # Use centralized retry helper From f6a5568865c21a2d59c32d3f7e52ebb8c5e0ec8a Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:37:38 -0700 Subject: [PATCH 07/21] telemetry: prefer config with env override; validate scheme; robust load\n\n- TelemetryConfig reads config.telemetry_enabled/endpoint, env can override\n- Validate endpoint scheme; revalidate on send\n- Split UUID/milestones load error handling\n- Add tests for config precedence, scheme validation, UUID preservation\n- validate_script: optional include_diagnostics with documented behavior --- .../UnityMcpServer~/src/telemetry.py | 54 ++++++++++++++---- .../src/tools/manage_script.py | 12 ++-- tests/test_telemetry_endpoint_validation.py | 56 +++++++++++++++++++ 3 files changed, 105 insertions(+), 17 deletions(-) create mode 100644 tests/test_telemetry_endpoint_validation.py diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 3141d1ba5..73991070f 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -17,6 +17,7 @@ from dataclasses import dataclass, asdict from typing import Optional, Dict, Any, List from pathlib import Path +import importlib try: import httpx @@ -61,11 +62,29 @@ class TelemetryRecord: class TelemetryConfig: """Telemetry configuration""" def __init__(self): - # Check environment variables for opt-out - self.enabled = not self._is_disabled() + # Prefer config file, then allow env overrides + server_config = None + for modname in ( + "UnityMcpBridge.UnityMcpServer~.src.config", + "UnityMcpBridge.UnityMcpServer.src.config", + "src.config", + "config", + ): + try: + mod = importlib.import_module(modname) + server_config = getattr(mod, "config", None) + if server_config is not None: + break + except Exception: + continue + + # Determine enabled flag: config -> env DISABLE_* opt-out + cfg_enabled = True if server_config is None else bool(getattr(server_config, "telemetry_enabled", True)) + self.enabled = cfg_enabled and not self._is_disabled() # Telemetry endpoint (Cloud Run default; override via env) - default_ep = "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" + cfg_default = None if server_config is None else getattr(server_config, "telemetry_endpoint", None) + default_ep = cfg_default or "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" self.default_endpoint = default_ep self.endpoint = self._validated_endpoint( os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep), @@ -142,20 +161,31 @@ def __init__(self): def _load_persistent_data(self): """Load UUID and milestones from disk""" + # Load customer UUID try: - # Load customer UUID if self.config.uuid_file.exists(): - self._customer_uuid = self.config.uuid_file.read_text().strip() + self._customer_uuid = self.config.uuid_file.read_text(encoding="utf-8").strip() or str(uuid.uuid4()) else: self._customer_uuid = str(uuid.uuid4()) - self.config.uuid_file.write_text(self._customer_uuid) - - # Load milestones - if self.config.milestones_file.exists(): - self._milestones = json.loads(self.config.milestones_file.read_text()) - except Exception as e: - logger.warning(f"Failed to load telemetry data: {e}") + try: + self.config.uuid_file.write_text(self._customer_uuid, encoding="utf-8") + if os.name == "posix": + os.chmod(self.config.uuid_file, 0o600) + except OSError as e: + logger.debug(f"Failed to persist customer UUID: {e}", exc_info=True) + except OSError as e: + logger.debug(f"Failed to load customer UUID: {e}", exc_info=True) self._customer_uuid = str(uuid.uuid4()) + + # Load milestones (failure here must not affect UUID) + try: + if self.config.milestones_file.exists(): + content = self.config.milestones_file.read_text(encoding="utf-8") + self._milestones = json.loads(content) or {} + if not isinstance(self._milestones, dict): + self._milestones = {} + except (OSError, json.JSONDecodeError, ValueError) as e: + logger.debug(f"Failed to load milestones: {e}", exc_info=True) self._milestones = {} def _save_milestones(self): diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index 149f06efe..99c33bbbc 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -409,13 +409,14 @@ def delete_script(ctx: Context, uri: str) -> Dict[str, Any]: @mcp.tool(description=( "Validate a C# script and return diagnostics.\n\n" - "Args: uri, level=('basic'|'standard').\n" + "Args: uri, level=('basic'|'standard'), include_diagnostics (bool, optional).\n" "- basic: quick syntax checks.\n" "- standard: deeper checks (performance hints, common pitfalls).\n" + "- include_diagnostics: when true, returns full diagnostics and summary; default returns counts only.\n" )) @telemetry_tool("validate_script") def validate_script( - ctx: Context, uri: str, level: str = "basic" + ctx: Context, uri: str, level: str = "basic", include_diagnostics: bool = False ) -> Dict[str, Any]: """Validate a C# script and return diagnostics.""" name, directory = _split_uri(uri) @@ -431,9 +432,11 @@ def validate_script( } resp = send_command_with_retry("manage_script", params) if isinstance(resp, dict) and resp.get("success"): - diags = resp.get("data", {}).get("diagnostics", []) - warnings = sum(1 for d in diags if str(d.get("severity", "")).lower() in ("warning",)) + diags = resp.get("data", {}).get("diagnostics", []) or [] + warnings = sum(1 for d in diags if str(d.get("severity", "")).lower() == "warning") errors = sum(1 for d in diags if str(d.get("severity", "")).lower() in ("error", "fatal")) + if include_diagnostics: + return {"success": True, "data": {"diagnostics": diags, "summary": {"warnings": warnings, "errors": errors}}} return {"success": True, "data": {"warnings": warnings, "errors": errors}} return resp if isinstance(resp, dict) else {"success": False, "message": str(resp)} @@ -573,7 +576,6 @@ def manage_script( @mcp.tool(description=( "Get manage_script capabilities (supported ops, limits, and guards).\n\n" - "Args:\n- random_string: required parameter (any string value)\n\n" "Returns:\n- ops: list of supported structured ops\n- text_ops: list of supported text ops\n- max_edit_payload_bytes: server edit payload cap\n- guards: header/using guard enabled flag\n" )) @telemetry_tool("manage_script_capabilities") diff --git a/tests/test_telemetry_endpoint_validation.py b/tests/test_telemetry_endpoint_validation.py new file mode 100644 index 000000000..8ba0d27b0 --- /dev/null +++ b/tests/test_telemetry_endpoint_validation.py @@ -0,0 +1,56 @@ +import os +import importlib + +def test_endpoint_rejects_non_http(tmp_path, monkeypatch): + # Point data dir to temp to avoid touching real files + monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) + monkeypatch.setenv("UNITY_MCP_TELEMETRY_ENDPOINT", "file:///etc/passwd") + + telemetry = importlib.import_module("UnityMcpBridge.UnityMcpServer~.src.telemetry") + importlib.reload(telemetry) + + tc = telemetry.TelemetryCollector() + # Should have fallen back to default endpoint + assert tc.config.endpoint == tc.config.default_endpoint + +def test_config_preferred_then_env_override(tmp_path, monkeypatch): + # Simulate config telemetry endpoint + monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) + monkeypatch.delenv("UNITY_MCP_TELEMETRY_ENDPOINT", raising=False) + + # Patch config.telemetry_endpoint via import mocking + import importlib + cfg_mod = importlib.import_module("UnityMcpBridge.UnityMcpServer~.src.config") + old_endpoint = cfg_mod.config.telemetry_endpoint + cfg_mod.config.telemetry_endpoint = "https://example.com/telemetry" + try: + telemetry = importlib.import_module("UnityMcpBridge.UnityMcpServer~.src.telemetry") + importlib.reload(telemetry) + tc = telemetry.TelemetryCollector() + assert tc.config.endpoint == "https://example.com/telemetry" + + # Env should override config + monkeypatch.setenv("UNITY_MCP_TELEMETRY_ENDPOINT", "https://override.example/ep") + importlib.reload(telemetry) + tc2 = telemetry.TelemetryCollector() + assert tc2.config.endpoint == "https://override.example/ep" + finally: + cfg_mod.config.telemetry_endpoint = old_endpoint + +def test_uuid_preserved_on_malformed_milestones(tmp_path, monkeypatch): + monkeypatch.setenv("XDG_DATA_HOME", str(tmp_path)) + + telemetry = importlib.import_module("UnityMcpBridge.UnityMcpServer~.src.telemetry") + importlib.reload(telemetry) + + tc1 = telemetry.TelemetryCollector() + first_uuid = tc1._customer_uuid + + # Write malformed milestones + tc1.config.milestones_file.write_text("{not-json}", encoding="utf-8") + + # Reload collector; UUID should remain same despite bad milestones + importlib.reload(telemetry) + tc2 = telemetry.TelemetryCollector() + assert tc2._customer_uuid == first_uuid + From 2abca24e9da8540e7870888b975898fd675c6af4 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:45:45 -0700 Subject: [PATCH 08/21] telemetry: pluggable Unity sender; add MCP_DISABLE_TELEMETRY; server reads version file; locks for milestones --- .../Editor/Helpers/TelemetryHelper.cs | 42 ++++++++++++++++--- .../UnityMcpServer~/src/server-version.txt | 1 + UnityMcpBridge/UnityMcpServer~/src/server.py | 23 ++++++++-- .../UnityMcpServer~/src/telemetry.py | 21 +++++----- 4 files changed, 66 insertions(+), 21 deletions(-) create mode 100644 UnityMcpBridge/UnityMcpServer~/src/server-version.txt diff --git a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs index 079ca6f59..d15af82f0 100644 --- a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs +++ b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs @@ -12,6 +12,7 @@ public static class TelemetryHelper { private const string TELEMETRY_DISABLED_KEY = "MCPForUnity.TelemetryDisabled"; private const string CUSTOMER_UUID_KEY = "MCPForUnity.CustomerUUID"; + private static Action> s_sender; /// /// Check if telemetry is enabled (can be disabled via Environment Variable or EditorPrefs) @@ -34,7 +35,15 @@ public static bool IsEnabled { return false; } - + + // Honor protocol-wide opt-out as well + var mcpDisable = Environment.GetEnvironmentVariable("MCP_DISABLE_TELEMETRY"); + if (!string.IsNullOrEmpty(mcpDisable) && + (mcpDisable.Equals("true", StringComparison.OrdinalIgnoreCase) || mcpDisable == "1")) + { + return false; + } + // Check EditorPrefs return !UnityEditor.EditorPrefs.GetBool(TELEMETRY_DISABLED_KEY, false); } @@ -110,6 +119,14 @@ public static void RecordEvent(string eventType, Dictionary data } } + /// + /// Allows the bridge to register a concrete sender for telemetry payloads. + /// + public static void RegisterTelemetrySender(Action> sender) + { + s_sender = sender; + } + /// /// Record bridge startup event /// @@ -162,15 +179,28 @@ public static void RecordToolExecution(string toolName, bool success, float dura private static void SendTelemetryToPythonServer(Dictionary telemetryData) { - // This would integrate with the existing bridge communication system - // For now, we'll just log it when debug is enabled + var sender = s_sender; + if (sender != null) + { + try + { + sender(telemetryData); + return; + } + catch (Exception e) + { + if (IsDebugEnabled()) + { + Debug.LogWarning($"Telemetry sender error (non-blocking): {e.Message}"); + } + } + } + + // Fallback: log when debug is enabled if (IsDebugEnabled()) { Debug.Log($"MCP-TELEMETRY: {telemetryData["event_type"]}"); } - - // TODO: Integrate with MCPForUnityBridge command system - // We would send this as a special telemetry command to the Python server } private static bool IsDebugEnabled() diff --git a/UnityMcpBridge/UnityMcpServer~/src/server-version.txt b/UnityMcpBridge/UnityMcpServer~/src/server-version.txt new file mode 100644 index 000000000..15a279981 --- /dev/null +++ b/UnityMcpBridge/UnityMcpServer~/src/server-version.txt @@ -0,0 +1 @@ +3.3.0 diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index 08570bb40..cda5f3fb7 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -27,8 +27,15 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: # Record server startup telemetry start_time = time.time() + start_clk = time.perf_counter() + try: + from pathlib import Path + ver_path = Path(__file__).parent / "server-version.txt" + server_version = ver_path.read_text(encoding="utf-8").strip() + except Exception: + server_version = "unknown" record_telemetry(RecordType.STARTUP, { - "server_version": "3.0.2", + "server_version": server_version, "startup_time": start_time }) @@ -45,15 +52,23 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: "connection_time_ms": (time.time() - start_time) * 1000 }) - except Exception as e: - logger.warning(f"Could not connect to Unity on startup: {str(e)}") + except ConnectionError as e: + logger.warning("Could not connect to Unity on startup: %s", e) _unity_connection = None # Record connection failure record_telemetry(RecordType.UNITY_CONNECTION, { "status": "failed", "error": str(e)[:200], - "connection_time_ms": (time.time() - start_time) * 1000 + "connection_time_ms": (time.perf_counter() - start_clk) * 1000 + }) + except Exception as e: + logger.warning("Unexpected error connecting to Unity on startup: %s", e) + _unity_connection = None + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "failed", + "error": str(e)[:200], + "connection_time_ms": (time.perf_counter() - start_clk) * 1000 }) try: diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 73991070f..fd9e82cac 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -157,6 +157,7 @@ def __init__(self): self.config = TelemetryConfig() self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} + self._lock: threading.Lock = threading.Lock() self._load_persistent_data() def _load_persistent_data(self): @@ -199,18 +200,16 @@ def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, An """Record a milestone event, returns True if this is the first occurrence""" if not self.config.enabled: return False - milestone_key = milestone.value - if milestone_key in self._milestones: - return False # Already recorded - - milestone_data = { - "timestamp": time.time(), - "data": data or {} - } - - self._milestones[milestone_key] = milestone_data - self._save_milestones() + with self._lock: + if milestone_key in self._milestones: + return False # Already recorded + milestone_data = { + "timestamp": time.time(), + "data": data or {}, + } + self._milestones[milestone_key] = milestone_data + self._save_milestones() # Also send as telemetry record self.record( From 2e907f189e0df9c8f374ff45abe1d98064c8e3d8 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 20:55:00 -0700 Subject: [PATCH 09/21] tests: disable telemetry during pytest via conftest; lock milestone saves --- UnityMcpBridge/UnityMcpServer~/src/telemetry.py | 10 +++++++--- tests/conftest.py | 8 ++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 tests/conftest.py diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index fd9e82cac..bc62b9582 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -192,9 +192,13 @@ def _load_persistent_data(self): def _save_milestones(self): """Save milestones to disk""" try: - self.config.milestones_file.write_text(json.dumps(self._milestones, indent=2)) - except Exception as e: - logger.warning(f"Failed to save milestones: {e}") + with self._lock: + self.config.milestones_file.write_text( + json.dumps(self._milestones, indent=2), + encoding="utf-8", + ) + except OSError as e: + logger.warning(f"Failed to save milestones: {e}", exc_info=True) def record_milestone(self, milestone: MilestoneType, data: Optional[Dict[str, Any]] = None) -> bool: """Record a milestone event, returns True if this is the first occurrence""" diff --git a/tests/conftest.py b/tests/conftest.py new file mode 100644 index 000000000..a839e9c4c --- /dev/null +++ b/tests/conftest.py @@ -0,0 +1,8 @@ +import os + +# Ensure telemetry is disabled during test collection and execution to avoid +# any background network or thread startup that could slow or block pytest. +os.environ.setdefault("DISABLE_TELEMETRY", "true") +os.environ.setdefault("UNITY_MCP_DISABLE_TELEMETRY", "true") +os.environ.setdefault("MCP_DISABLE_TELEMETRY", "true") + From ba45051a402fed20596a17892a9f91aba7290c49 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Mon, 8 Sep 2025 21:37:06 -0700 Subject: [PATCH 10/21] fix she descrip --- UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py index 99c33bbbc..0c27eb087 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_script.py @@ -602,9 +602,9 @@ def manage_script_capabilities(ctx: Context) -> Dict[str, Any]: return {"success": False, "error": f"capabilities error: {e}"} @mcp.tool(description=( - "Get SHA256 and metadata for a Unity C# script without returning file contents.\n\n" + "Get SHA256 and basic metadata for a Unity C# script without returning file contents.\n\n" "Args: uri (unity://path/Assets/... or file://... or Assets/...).\n" - "Returns: {sha256, lengthBytes, lastModifiedUtc, uri, path}." + "Returns: {sha256, lengthBytes}." )) @telemetry_tool("get_sha") def get_sha(ctx: Context, uri: str) -> Dict[str, Any]: From bd55a56d1c7927b6b10245f5bae786ff0ff86952 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Tue, 9 Sep 2025 12:14:00 -0700 Subject: [PATCH 11/21] MCP server: hardened startup + telemetry queue - Logging to stderr with force; quiet httpx/urllib3 - Async lifespan fix; defer telemetry in first second - Bounded telemetry queue with single worker - Reduce initial Unity connect timeout to 1s - Keep server_version in file --- UnityMcpBridge/UnityMcpServer~/src/config.py | 2 +- UnityMcpBridge/UnityMcpServer~/src/server.py | 97 +++++++++++-------- .../UnityMcpServer~/src/server_version.txt | 2 +- .../UnityMcpServer~/src/telemetry.py | 30 ++++-- 4 files changed, 84 insertions(+), 47 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/config.py b/UnityMcpBridge/UnityMcpServer~/src/config.py index e41b2724f..0f22fed31 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/config.py +++ b/UnityMcpBridge/UnityMcpServer~/src/config.py @@ -15,7 +15,7 @@ class ServerConfig: mcp_port: int = 6500 # Connection settings - connection_timeout: float = 60.0 # default steady-state timeout; retries use shorter timeouts + connection_timeout: float = 1.0 # short initial timeout; retries use shorter timeouts buffer_size: int = 16 * 1024 * 1024 # 16MB buffer # Framed receive behavior framed_receive_timeout: float = 2.0 # max seconds to wait while consuming heartbeats only diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index cda5f3fb7..26b824f46 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -1,20 +1,31 @@ from mcp.server.fastmcp import FastMCP, Context, Image import logging +import os from dataclasses import dataclass from contextlib import asynccontextmanager from typing import AsyncIterator, Dict, Any, List from config import config from tools import register_all_tools from unity_connection import get_unity_connection, UnityConnection -from telemetry import record_telemetry, record_milestone, RecordType, MilestoneType import time # Configure logging using settings from config logging.basicConfig( level=getattr(logging, config.log_level), - format=config.log_format + format=config.log_format, + stream=None, # None -> defaults to sys.stderr; avoid stdout used by MCP stdio + force=True # Ensure our handler replaces any prior stdout handlers ) logger = logging.getLogger("mcp-for-unity-server") +# Quieten noisy third-party loggers to avoid clutter during stdio handshake +for noisy in ("httpx", "urllib3"): + try: + logging.getLogger(noisy).setLevel(max(logging.WARNING, getattr(logging, config.log_level))) + except Exception: + pass + +# Import telemetry only after logging is configured to ensure its logs use stderr and proper levels +from telemetry import record_telemetry, record_milestone, RecordType, MilestoneType # Global connection state _unity_connection: UnityConnection = None @@ -34,42 +45,52 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: server_version = ver_path.read_text(encoding="utf-8").strip() except Exception: server_version = "unknown" - record_telemetry(RecordType.STARTUP, { - "server_version": server_version, - "startup_time": start_time - }) + # Defer telemetry for first second to avoid interfering with stdio handshake + if (time.perf_counter() - start_clk) > 1.0: + record_telemetry(RecordType.STARTUP, { + "server_version": server_version, + "startup_time": start_time + }) # Record first startup milestone - record_milestone(MilestoneType.FIRST_STARTUP) + if (time.perf_counter() - start_clk) > 1.0: + record_milestone(MilestoneType.FIRST_STARTUP) try: - _unity_connection = get_unity_connection() - logger.info("Connected to Unity on startup") - - # Record successful Unity connection - record_telemetry(RecordType.UNITY_CONNECTION, { - "status": "connected", - "connection_time_ms": (time.time() - start_time) * 1000 - }) - + skip_connect = os.environ.get("UNITY_MCP_SKIP_STARTUP_CONNECT", "").lower() in ("1", "true", "yes", "on") + if skip_connect: + logger.info("Skipping Unity connection on startup (UNITY_MCP_SKIP_STARTUP_CONNECT=1)") + else: + _unity_connection = get_unity_connection() + logger.info("Connected to Unity on startup") + + # Record successful Unity connection + if (time.perf_counter() - start_clk) > 1.0: + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "connected", + "connection_time_ms": (time.time() - start_time) * 1000 + }) + except ConnectionError as e: logger.warning("Could not connect to Unity on startup: %s", e) _unity_connection = None # Record connection failure - record_telemetry(RecordType.UNITY_CONNECTION, { - "status": "failed", - "error": str(e)[:200], - "connection_time_ms": (time.perf_counter() - start_clk) * 1000 - }) + if (time.perf_counter() - start_clk) > 1.0: + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "failed", + "error": str(e)[:200], + "connection_time_ms": (time.perf_counter() - start_clk) * 1000 + }) except Exception as e: logger.warning("Unexpected error connecting to Unity on startup: %s", e) _unity_connection = None - record_telemetry(RecordType.UNITY_CONNECTION, { - "status": "failed", - "error": str(e)[:200], - "connection_time_ms": (time.perf_counter() - start_clk) * 1000 - }) + if (time.perf_counter() - start_clk) > 1.0: + record_telemetry(RecordType.UNITY_CONNECTION, { + "status": "failed", + "error": str(e)[:200], + "connection_time_ms": (time.perf_counter() - start_clk) * 1000 + }) try: # Yield the connection object so it can be attached to the context @@ -97,18 +118,18 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: def asset_creation_strategy() -> str: """Guide for discovering and using MCP for Unity tools effectively.""" return ( - "Available MCP for Unity Server Tools:\\n\\n" - "- `manage_editor`: Controls editor state and queries info.\\n" - "- `execute_menu_item`: Executes Unity Editor menu items by path.\\n" - "- `read_console`: Reads or clears Unity console messages, with filtering options.\\n" - "- `manage_scene`: Manages scenes.\\n" - "- `manage_gameobject`: Manages GameObjects in the scene.\\n" - "- `manage_script`: Manages C# script files.\\n" - "- `manage_asset`: Manages prefabs and assets.\\n" - "- `manage_shader`: Manages shaders.\\n\\n" - "Tips:\\n" - "- Create prefabs for reusable GameObjects.\\n" - "- Always include a camera and main light in your scenes.\\n" + "Available MCP for Unity Server Tools:\n\n" + "- `manage_editor`: Controls editor state and queries info.\n" + "- `execute_menu_item`: Executes Unity Editor menu items by path.\n" + "- `read_console`: Reads or clears Unity console messages, with filtering options.\n" + "- `manage_scene`: Manages scenes.\n" + "- `manage_gameobject`: Manages GameObjects in the scene.\n" + "- `manage_script`: Manages C# script files.\n" + "- `manage_asset`: Manages prefabs and assets.\n" + "- `manage_shader`: Manages shaders.\n\n" + "Tips:\n" + "- Create prefabs for reusable GameObjects.\n" + "- Always include a camera and main light in your scenes.\n" ) # Run the server diff --git a/UnityMcpBridge/UnityMcpServer~/src/server_version.txt b/UnityMcpBridge/UnityMcpServer~/src/server_version.txt index 15a279981..bea438e9a 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server_version.txt +++ b/UnityMcpBridge/UnityMcpServer~/src/server_version.txt @@ -1 +1 @@ -3.3.0 +3.3.1 diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index bc62b9582..8aed4cee9 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -18,6 +18,8 @@ from typing import Optional, Dict, Any, List from pathlib import Path import importlib +import queue +import contextlib try: import httpx @@ -158,6 +160,10 @@ def __init__(self): self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} self._lock: threading.Lock = threading.Lock() + # Bounded queue with single background worker to avoid spawning a thread per event + self._queue: "queue.Queue[tuple[contextvars.Context, TelemetryRecord]]" = queue.Queue(maxsize=1000) + self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) + self._worker.start() self._load_persistent_data() def _load_persistent_data(self): @@ -242,14 +248,24 @@ def record(self, data=data, milestone=milestone ) - - # Send in background thread to avoid blocking + # Enqueue for background worker (non-blocking). Drop on backpressure. current_context = contextvars.copy_context() - thread = threading.Thread( - target=lambda: current_context.run(self._send_telemetry, record), - daemon=True - ) - thread.start() + try: + self._queue.put_nowait((current_context, record)) + except queue.Full: + logger.debug("Telemetry queue full; dropping %s", record.record_type) + + def _worker_loop(self): + """Background worker that serializes telemetry sends.""" + while True: + ctx, rec = self._queue.get() + try: + ctx.run(self._send_telemetry, rec) + except Exception: + logger.debug("Telemetry worker send failed", exc_info=True) + finally: + with contextlib.suppress(Exception): + self._queue.task_done() def _send_telemetry(self, record: TelemetryRecord): """Send telemetry data to endpoint""" From 1e003748d8557c4b276e7cc894c9e826d49aa18a Mon Sep 17 00:00:00 2001 From: David Sarno Date: Tue, 9 Sep 2025 18:45:09 -0700 Subject: [PATCH 12/21] telemetry: bounded queue + single worker; INFO-level send logs; endpoint to Cloud Run; add unit test for backpressure --- .../UnityMcpServer~/src/telemetry.py | 10 +-- tests/test_telemetry_queue_worker.py | 79 +++++++++++++++++++ 2 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 tests/test_telemetry_queue_worker.py diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 8aed4cee9..8d6be46ac 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -302,9 +302,9 @@ def _send_telemetry(self, record: TelemetryRecord): endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint) response = client.post(endpoint, json=payload) if response.status_code == 200: - logger.debug(f"Telemetry sent: {record.record_type}") + logger.info(f"Telemetry sent: {record.record_type}") else: - logger.debug(f"Telemetry failed: HTTP {response.status_code}") + logger.warning(f"Telemetry failed: HTTP {response.status_code}") else: import urllib.request import urllib.error @@ -319,11 +319,11 @@ def _send_telemetry(self, record: TelemetryRecord): try: with urllib.request.urlopen(req, timeout=self.config.timeout) as resp: if 200 <= resp.getcode() < 300: - logger.debug(f"Telemetry sent (urllib): {record.record_type}") + logger.info(f"Telemetry sent (urllib): {record.record_type}") else: - logger.debug(f"Telemetry failed (urllib): HTTP {resp.getcode()}") + logger.warning(f"Telemetry failed (urllib): HTTP {resp.getcode()}") except urllib.error.URLError as ue: - logger.debug(f"Telemetry send failed (urllib): {ue}") + logger.warning(f"Telemetry send failed (urllib): {ue}") except Exception as e: # Never let telemetry errors interfere with app functionality diff --git a/tests/test_telemetry_queue_worker.py b/tests/test_telemetry_queue_worker.py new file mode 100644 index 000000000..c3e372279 --- /dev/null +++ b/tests/test_telemetry_queue_worker.py @@ -0,0 +1,79 @@ +import sys +import pathlib +import importlib.util +import types +import threading +import time +import queue as q + + +ROOT = pathlib.Path(__file__).resolve().parents[1] +SRC = ROOT / "UnityMcpBridge" / "UnityMcpServer~" / "src" +sys.path.insert(0, str(SRC)) + +# Stub mcp.server.fastmcp to satisfy imports without the full dependency +mcp_pkg = types.ModuleType("mcp") +server_pkg = types.ModuleType("mcp.server") +fastmcp_pkg = types.ModuleType("mcp.server.fastmcp") + +class _Dummy: + pass + +fastmcp_pkg.FastMCP = _Dummy +fastmcp_pkg.Context = _Dummy +server_pkg.fastmcp = fastmcp_pkg +mcp_pkg.server = server_pkg +sys.modules.setdefault("mcp", mcp_pkg) +sys.modules.setdefault("mcp.server", server_pkg) +sys.modules.setdefault("mcp.server.fastmcp", fastmcp_pkg) + + +def _load_module(path: pathlib.Path, name: str): + spec = importlib.util.spec_from_file_location(name, path) + mod = importlib.util.module_from_spec(spec) + spec.loader.exec_module(mod) + return mod + + +telemetry = _load_module(SRC / "telemetry.py", "telemetry_mod") + + +def test_telemetry_queue_backpressure_and_single_worker(monkeypatch, caplog): + caplog.set_level("DEBUG") + + collector = telemetry.TelemetryCollector() + # Force-enable telemetry regardless of env settings from conftest + collector.config.enabled = True + + # Replace queue with tiny one to trigger backpressure quickly + small_q = q.Queue(maxsize=2) + collector._queue = small_q + + # Make sends slow to build backlog and exercise worker + def slow_send(self, rec): + time.sleep(0.05) + + collector._send_telemetry = types.MethodType(slow_send, collector) + + # Fire many events quickly; record() should not block even when queue fills + start = time.perf_counter() + for i in range(50): + collector.record(telemetry.RecordType.TOOL_EXECUTION, {"i": i}) + elapsed_ms = (time.perf_counter() - start) * 1000.0 + + # Should be fast despite backpressure (non-blocking enqueue or drop) + assert elapsed_ms < 80.0 + + # Allow worker to process some + time.sleep(0.3) + + # Verify drops were logged (queue full backpressure) + dropped_logs = [m for m in caplog.messages if "Telemetry queue full; dropping" in m] + assert len(dropped_logs) >= 1 + + # Ensure only one worker thread exists and is alive + assert collector._worker.is_alive() + worker_threads = [t for t in threading.enumerate() if t is collector._worker] + assert len(worker_threads) == 1 + + From c1bde804d4c22151a05da4fee2d083974ea036e4 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Tue, 9 Sep 2025 18:46:42 -0700 Subject: [PATCH 13/21] telemetry: main-thread routing + timeout for manage_scene; stderr + rotating file logs; Cloud Run endpoint in config; minor robustness in scene tool --- UnityMcpBridge/Editor/MCPForUnityBridge.cs | 94 ++++++++++++++++++- UnityMcpBridge/UnityMcpServer~/src/config.py | 2 +- UnityMcpBridge/UnityMcpServer~/src/server.py | 15 +++ .../UnityMcpServer~/src/tools/manage_scene.py | 2 +- 4 files changed, 109 insertions(+), 4 deletions(-) diff --git a/UnityMcpBridge/Editor/MCPForUnityBridge.cs b/UnityMcpBridge/Editor/MCPForUnityBridge.cs index ef6685528..5ee3c0c78 100644 --- a/UnityMcpBridge/Editor/MCPForUnityBridge.cs +++ b/UnityMcpBridge/Editor/MCPForUnityBridge.cs @@ -38,6 +38,7 @@ private static Dictionary< string, (string commandJson, TaskCompletionSource tcs) > commandQueue = new(); + private static int mainThreadId; private static int currentUnityPort = 6400; // Dynamic port, starts with default private static bool isAutoConnectMode = false; private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads @@ -109,6 +110,8 @@ public static bool FolderExists(string path) static MCPForUnityBridge() { + // Record the main thread ID for safe thread checks + try { mainThreadId = Thread.CurrentThread.ManagedThreadId; } catch { mainThreadId = 0; } // Skip bridge in headless/batch environments (CI/builds) unless explicitly allowed via env // CI override: set UNITY_MCP_ALLOW_BATCH=1 to allow the bridge in batch mode if (Application.isBatchMode && string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("UNITY_MCP_ALLOW_BATCH"))) @@ -539,7 +542,39 @@ private static async Task HandleClientAsync(TcpClient client, CancellationToken commandQueue[commandId] = (commandText, tcs); } - string response = await tcs.Task.ConfigureAwait(false); + // Wait for the handler to produce a response, but do not block indefinitely + string response; + try + { + using var respCts = new CancellationTokenSource(FrameIOTimeoutMs); + var completed = await Task.WhenAny(tcs.Task, Task.Delay(FrameIOTimeoutMs, respCts.Token)).ConfigureAwait(false); + if (completed == tcs.Task) + { + // Got a result from the handler + respCts.Cancel(); + response = tcs.Task.Result; + } + else + { + // Timeout: return a structured error so the client can recover + var timeoutResponse = new + { + status = "error", + error = $"Command processing timed out after {FrameIOTimeoutMs} ms", + }; + response = JsonConvert.SerializeObject(timeoutResponse); + } + } + catch (Exception ex) + { + var errorResponse = new + { + status = "error", + error = ex.Message, + }; + response = JsonConvert.SerializeObject(errorResponse); + } + byte[] responseBytes = System.Text.Encoding.UTF8.GetBytes(response); await WriteFrameAsync(stream, responseBytes); } @@ -816,6 +851,60 @@ private static void ProcessCommands() } } + // Invoke the given function on the Unity main thread and wait up to timeoutMs for the result. + // Returns null on timeout or error; caller should provide a fallback error response. + private static object InvokeOnMainThreadWithTimeout(Func func, int timeoutMs) + { + if (func == null) return null; + try + { + // If we are already on the main thread, execute directly to avoid deadlocks + try + { + if (Thread.CurrentThread.ManagedThreadId == mainThreadId) + { + return func(); + } + } + catch { } + + object result = null; + Exception captured = null; + var tcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + EditorApplication.delayCall += () => + { + try + { + result = func(); + } + catch (Exception ex) + { + captured = ex; + } + finally + { + try { tcs.TrySetResult(true); } catch { } + } + }; + + // Wait for completion with timeout (Editor thread will pump delayCall) + bool completed = tcs.Task.Wait(timeoutMs); + if (!completed) + { + return null; // timeout + } + if (captured != null) + { + return Response.Error($"Main thread handler error: {captured.Message}"); + } + return result; + } + catch (Exception ex) + { + return Response.Error($"Failed to invoke on main thread: {ex.Message}"); + } + } + // Helper method to check if a string is valid JSON private static bool IsValidJson(string text) { @@ -880,7 +969,8 @@ private static string ExecuteCommand(Command command) // Maps the command type (tool name) to the corresponding handler's static HandleCommand method // Assumes each handler class has a static method named 'HandleCommand' that takes JObject parameters "manage_script" => ManageScript.HandleCommand(paramsObject), - "manage_scene" => ManageScene.HandleCommand(paramsObject), + // Run scene operations on the main thread to avoid deadlocks/hangs + "manage_scene" => InvokeOnMainThreadWithTimeout(() => ManageScene.HandleCommand(paramsObject), FrameIOTimeoutMs) ?? Response.Error("manage_scene timed out on main thread"), "manage_editor" => ManageEditor.HandleCommand(paramsObject), "manage_gameobject" => ManageGameObject.HandleCommand(paramsObject), "manage_asset" => ManageAsset.HandleCommand(paramsObject), diff --git a/UnityMcpBridge/UnityMcpServer~/src/config.py b/UnityMcpBridge/UnityMcpServer~/src/config.py index 0f22fed31..5d0556725 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/config.py +++ b/UnityMcpBridge/UnityMcpServer~/src/config.py @@ -36,7 +36,7 @@ class ServerConfig: # Telemetry settings telemetry_enabled: bool = True - telemetry_endpoint: str = "https://api-prod.coplay.dev/telemetry/events" + telemetry_endpoint: str = "https://unity-mcp-telemetry-a6uvvbgbsa-uc.a.run.app/telemetry/events" # Create a global config instance config = ServerConfig() \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index 26b824f46..da5ae9487 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -1,5 +1,6 @@ from mcp.server.fastmcp import FastMCP, Context, Image import logging +from logging.handlers import RotatingFileHandler import os from dataclasses import dataclass from contextlib import asynccontextmanager @@ -17,6 +18,20 @@ force=True # Ensure our handler replaces any prior stdout handlers ) logger = logging.getLogger("mcp-for-unity-server") + +# Also write logs to a rotating file so logs are available when launched via stdio +try: + import os as _os + _log_dir = _os.path.join(_os.path.expanduser("~/Library/Application Support/UnityMCP"), "Logs") + _os.makedirs(_log_dir, exist_ok=True) + _file_path = _os.path.join(_log_dir, "unity_mcp_server.log") + _fh = RotatingFileHandler(_file_path, maxBytes=512*1024, backupCount=2, encoding="utf-8") + _fh.setFormatter(logging.Formatter(config.log_format)) + _fh.setLevel(getattr(logging, config.log_level)) + logger.addHandler(_fh) +except Exception: + # Never let logging setup break startup + pass # Quieten noisy third-party loggers to avoid clutter during stdio handshake for noisy in ("httpx", "urllib3"): try: diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py index ef9272528..67caa64b4 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py @@ -12,7 +12,7 @@ def register_manage_scene_tools(mcp: FastMCP): @mcp.tool() @telemetry_tool("manage_scene") def manage_scene( - ctx: Any, + ctx: Context, action: str, name: str, path: str, From 46df7250b514b34b5a645edc0915d2964728e695 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Tue, 9 Sep 2025 19:26:17 -0700 Subject: [PATCH 14/21] attempted ManageScene debugging for hang --- UnityMcpBridge/Editor/MCPForUnityBridge.cs | 36 +++++++++++++++++++--- UnityMcpBridge/Editor/Tools/ManageScene.cs | 24 +++++++++++++-- 2 files changed, 53 insertions(+), 7 deletions(-) diff --git a/UnityMcpBridge/Editor/MCPForUnityBridge.cs b/UnityMcpBridge/Editor/MCPForUnityBridge.cs index 5ee3c0c78..23bdc12c3 100644 --- a/UnityMcpBridge/Editor/MCPForUnityBridge.cs +++ b/UnityMcpBridge/Editor/MCPForUnityBridge.cs @@ -575,6 +575,10 @@ private static async Task HandleClientAsync(TcpClient client, CancellationToken response = JsonConvert.SerializeObject(errorResponse); } + if (IsDebugEnabled()) + { + try { MCPForUnity.Editor.Helpers.McpLog.Info("[MCP] sending framed response", always: false); } catch { } + } byte[] responseBytes = System.Text.Encoding.UTF8.GetBytes(response); await WriteFrameAsync(stream, responseBytes); } @@ -858,6 +862,12 @@ private static object InvokeOnMainThreadWithTimeout(Func func, int timeo if (func == null) return null; try { + // If mainThreadId is unknown, assume we're on main thread to avoid blocking the editor. + if (mainThreadId == 0) + { + try { return func(); } + catch (Exception ex) { throw new InvalidOperationException($"Main thread handler error: {ex.Message}", ex); } + } // If we are already on the main thread, execute directly to avoid deadlocks try { @@ -895,13 +905,13 @@ private static object InvokeOnMainThreadWithTimeout(Func func, int timeo } if (captured != null) { - return Response.Error($"Main thread handler error: {captured.Message}"); + throw new InvalidOperationException($"Main thread handler error: {captured.Message}", captured); } return result; } catch (Exception ex) { - return Response.Error($"Failed to invoke on main thread: {ex.Message}"); + throw new InvalidOperationException($"Failed to invoke on main thread: {ex.Message}", ex); } } @@ -969,8 +979,9 @@ private static string ExecuteCommand(Command command) // Maps the command type (tool name) to the corresponding handler's static HandleCommand method // Assumes each handler class has a static method named 'HandleCommand' that takes JObject parameters "manage_script" => ManageScript.HandleCommand(paramsObject), - // Run scene operations on the main thread to avoid deadlocks/hangs - "manage_scene" => InvokeOnMainThreadWithTimeout(() => ManageScene.HandleCommand(paramsObject), FrameIOTimeoutMs) ?? Response.Error("manage_scene timed out on main thread"), + // Run scene operations on the main thread to avoid deadlocks/hangs (with diagnostics under debug flag) + "manage_scene" => HandleManageScene(paramsObject) + ?? throw new TimeoutException($"manage_scene timed out after {FrameIOTimeoutMs} ms on main thread"), "manage_editor" => ManageEditor.HandleCommand(paramsObject), "manage_gameobject" => ManageGameObject.HandleCommand(paramsObject), "manage_asset" => ManageAsset.HandleCommand(paramsObject), @@ -1008,6 +1019,23 @@ private static string ExecuteCommand(Command command) } } + private static object HandleManageScene(JObject paramsObject) + { + try + { + if (IsDebugEnabled()) Debug.Log("[MCP] manage_scene: dispatching to main thread"); + var sw = System.Diagnostics.Stopwatch.StartNew(); + var r = InvokeOnMainThreadWithTimeout(() => ManageScene.HandleCommand(paramsObject), FrameIOTimeoutMs); + sw.Stop(); + if (IsDebugEnabled()) Debug.Log($"[MCP] manage_scene: completed in {sw.ElapsedMilliseconds} ms"); + return r ?? Response.Error("manage_scene returned null (timeout or error)"); + } + catch (Exception ex) + { + return Response.Error($"manage_scene dispatch error: {ex.Message}"); + } + } + // Helper method to get a summary of parameters for error reporting private static string GetParamsSummary(JObject @params) { diff --git a/UnityMcpBridge/Editor/Tools/ManageScene.cs b/UnityMcpBridge/Editor/Tools/ManageScene.cs index fbf0b7e02..d3ae3d153 100644 --- a/UnityMcpBridge/Editor/Tools/ManageScene.cs +++ b/UnityMcpBridge/Editor/Tools/ManageScene.cs @@ -21,6 +21,7 @@ public static class ManageScene /// public static object HandleCommand(JObject @params) { + try { McpLog.Info("[ManageScene] HandleCommand: start", always: false); } catch { } string action = @params["action"]?.ToString().ToLower(); string name = @params["name"]?.ToString(); string path = @params["path"]?.ToString(); // Relative to Assets/ @@ -76,6 +77,7 @@ public static object HandleCommand(JObject @params) } // Route action + try { McpLog.Info($"[ManageScene] Route action='{action}' name='{name}' path='{path}' buildIndex={(buildIndex.HasValue ? buildIndex.Value.ToString() : "null")}", always: false); } catch { } switch (action) { case "create": @@ -98,9 +100,15 @@ public static object HandleCommand(JObject @params) // Save current scene, optionally to a new path return SaveScene(fullPath, relativePath); case "get_hierarchy": - return GetSceneHierarchy(); + try { McpLog.Info("[ManageScene] get_hierarchy: entering", always: false); } catch { } + var gh = GetSceneHierarchy(); + try { McpLog.Info("[ManageScene] get_hierarchy: exiting", always: false); } catch { } + return gh; case "get_active": - return GetActiveSceneInfo(); + try { McpLog.Info("[ManageScene] get_active: entering", always: false); } catch { } + var ga = GetActiveSceneInfo(); + try { McpLog.Info("[ManageScene] get_active: exiting", always: false); } catch { } + return ga; case "get_build_settings": return GetBuildSettingsScenes(); // Add cases for modifying build settings, additive loading, unloading etc. @@ -294,7 +302,9 @@ private static object GetActiveSceneInfo() { try { + try { McpLog.Info("[ManageScene] get_active: querying EditorSceneManager.GetActiveScene", always: false); } catch { } Scene activeScene = EditorSceneManager.GetActiveScene(); + try { McpLog.Info($"[ManageScene] get_active: got scene valid={activeScene.IsValid()} loaded={activeScene.isLoaded} name='{activeScene.name}'", always: false); } catch { } if (!activeScene.IsValid()) { return Response.Error("No active scene found."); @@ -314,6 +324,7 @@ private static object GetActiveSceneInfo() } catch (Exception e) { + try { McpLog.Error($"[ManageScene] get_active: exception {e.Message}"); } catch { } return Response.Error($"Error getting active scene info: {e.Message}"); } } @@ -348,7 +359,9 @@ private static object GetSceneHierarchy() { try { + try { McpLog.Info("[ManageScene] get_hierarchy: querying EditorSceneManager.GetActiveScene", always: false); } catch { } Scene activeScene = EditorSceneManager.GetActiveScene(); + try { McpLog.Info($"[ManageScene] get_hierarchy: got scene valid={activeScene.IsValid()} loaded={activeScene.isLoaded} name='{activeScene.name}'", always: false); } catch { } if (!activeScene.IsValid() || !activeScene.isLoaded) { return Response.Error( @@ -356,16 +369,21 @@ private static object GetSceneHierarchy() ); } + try { McpLog.Info("[ManageScene] get_hierarchy: fetching root objects", always: false); } catch { } GameObject[] rootObjects = activeScene.GetRootGameObjects(); + try { McpLog.Info($"[ManageScene] get_hierarchy: rootCount={rootObjects?.Length ?? 0}", always: false); } catch { } var hierarchy = rootObjects.Select(go => GetGameObjectDataRecursive(go)).ToList(); - return Response.Success( + var resp = Response.Success( $"Retrieved hierarchy for scene '{activeScene.name}'.", hierarchy ); + try { McpLog.Info("[ManageScene] get_hierarchy: success", always: false); } catch { } + return resp; } catch (Exception e) { + try { McpLog.Error($"[ManageScene] get_hierarchy: exception {e.Message}"); } catch { } return Response.Error($"Error getting scene hierarchy: {e.Message}"); } } From 397ba32a99bba299860da098e78d71c3266e700a Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 10 Sep 2025 07:50:05 -0700 Subject: [PATCH 15/21] telemetry: fire-and-forget queue; remove context propagation; reduce timeouts; fix milestone lock reentrancy --- .../UnityMcpServer~/src/telemetry.py | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 8d6be46ac..5e2031acf 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -5,7 +5,11 @@ import uuid import threading -import contextvars +""" +Fire-and-forget telemetry sender with a single background worker. +- No context/thread-local propagation to avoid re-entrancy into tool resolution. +- Small network timeouts to prevent stalls. +""" import json import time import os @@ -98,8 +102,11 @@ def __init__(self): self.uuid_file = self.data_dir / "customer_uuid.txt" self.milestones_file = self.data_dir / "milestones.json" - # Request timeout - self.timeout = 10.0 + # Request timeout (small, fail fast). Override with UNITY_MCP_TELEMETRY_TIMEOUT + try: + self.timeout = float(os.environ.get("UNITY_MCP_TELEMETRY_TIMEOUT", "1.5")) + except Exception: + self.timeout = 1.5 # Session tracking self.session_id = str(uuid.uuid4()) @@ -160,8 +167,8 @@ def __init__(self): self._customer_uuid: Optional[str] = None self._milestones: Dict[str, Dict[str, Any]] = {} self._lock: threading.Lock = threading.Lock() - # Bounded queue with single background worker to avoid spawning a thread per event - self._queue: "queue.Queue[tuple[contextvars.Context, TelemetryRecord]]" = queue.Queue(maxsize=1000) + # Bounded queue with single background worker (records only; no context propagation) + self._queue: "queue.Queue[TelemetryRecord]" = queue.Queue(maxsize=1000) self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) self._worker.start() self._load_persistent_data() @@ -196,13 +203,12 @@ def _load_persistent_data(self): self._milestones = {} def _save_milestones(self): - """Save milestones to disk""" + """Save milestones to disk. Caller must hold self._lock.""" try: - with self._lock: - self.config.milestones_file.write_text( - json.dumps(self._milestones, indent=2), - encoding="utf-8", - ) + self.config.milestones_file.write_text( + json.dumps(self._milestones, indent=2), + encoding="utf-8", + ) except OSError as e: logger.warning(f"Failed to save milestones: {e}", exc_info=True) @@ -249,18 +255,18 @@ def record(self, milestone=milestone ) # Enqueue for background worker (non-blocking). Drop on backpressure. - current_context = contextvars.copy_context() try: - self._queue.put_nowait((current_context, record)) + self._queue.put_nowait(record) except queue.Full: logger.debug("Telemetry queue full; dropping %s", record.record_type) def _worker_loop(self): """Background worker that serializes telemetry sends.""" while True: - ctx, rec = self._queue.get() + rec = self._queue.get() try: - ctx.run(self._send_telemetry, rec) + # Run sender directly; do not reuse caller context/thread-locals + self._send_telemetry(rec) except Exception: logger.debug("Telemetry worker send failed", exc_info=True) finally: From 2fd74f5dab18c426b8d8a3dc6aa99ff229a658b8 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 10 Sep 2025 08:23:25 -0700 Subject: [PATCH 16/21] manage_scene: tolerant params + optional buildIndex; add writer IO logs; keep direct write path --- UnityMcpBridge/Editor/MCPForUnityBridge.cs | 70 ++++++++++++++++++- UnityMcpBridge/Editor/Tools/ManageScene.cs | 38 ++++++++-- .../UnityMcpServer~/src/tools/manage_scene.py | 22 +++--- 3 files changed, 113 insertions(+), 17 deletions(-) diff --git a/UnityMcpBridge/Editor/MCPForUnityBridge.cs b/UnityMcpBridge/Editor/MCPForUnityBridge.cs index 23bdc12c3..2c92b9ce8 100644 --- a/UnityMcpBridge/Editor/MCPForUnityBridge.cs +++ b/UnityMcpBridge/Editor/MCPForUnityBridge.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Collections.Concurrent; using System.IO; using System.Linq; using System.Net; @@ -25,6 +26,14 @@ public static partial class MCPForUnityBridge private static readonly object startStopLock = new(); private static readonly object clientsLock = new(); private static readonly System.Collections.Generic.HashSet activeClients = new(); + // Single-writer outbox for framed responses + private class Outbound + { + public byte[] Payload; + public string Tag; + public int? ReqId; + } + private static readonly BlockingCollection _outbox = new(new ConcurrentQueue()); private static CancellationTokenSource cts; private static Task listenerTask; private static int processingCommands = 0; @@ -44,6 +53,10 @@ private static Dictionary< private const ulong MaxFrameBytes = 64UL * 1024 * 1024; // 64 MiB hard cap for framed payloads private const int FrameIOTimeoutMs = 30000; // Per-read timeout to avoid stalled clients + // IO diagnostics + private static long _ioSeq = 0; + private static void IoInfo(string s) { McpLog.Info(s, always: false); } + // Debug helpers private static bool IsDebugEnabled() { @@ -112,6 +125,35 @@ static MCPForUnityBridge() { // Record the main thread ID for safe thread checks try { mainThreadId = Thread.CurrentThread.ManagedThreadId; } catch { mainThreadId = 0; } + // Start single writer thread for framed responses + try + { + var writerThread = new Thread(() => + { + foreach (var item in _outbox.GetConsumingEnumerable()) + { + try + { + long seq = Interlocked.Increment(ref _ioSeq); + IoInfo($"[IO] โžœ write start seq={seq} tag={item.Tag} len={(item.Payload?.Length ?? 0)} reqId={(item.ReqId?.ToString() ?? "?")}"); + var sw = System.Diagnostics.Stopwatch.StartNew(); + // Note: We currently have a per-connection 'stream' in the client handler. For simplicity, + // writes are performed inline there. This outbox provides single-writer semantics; if a shared + // stream is introduced, redirect here accordingly. + // No-op: actual write happens in client loop using WriteFrameAsync + sw.Stop(); + IoInfo($"[IO] โœ“ write end tag={item.Tag} len={(item.Payload?.Length ?? 0)} reqId={(item.ReqId?.ToString() ?? "?")} durMs={sw.Elapsed.TotalMilliseconds:F1}"); + } + catch (Exception ex) + { + IoInfo($"[IO] โœ— write FAIL tag={item.Tag} reqId={(item.ReqId?.ToString() ?? "?")} {ex.GetType().Name}: {ex.Message}"); + } + } + }) { IsBackground = true, Name = "MCP-Writer" }; + writerThread.Start(); + } + catch { } + // Skip bridge in headless/batch environments (CI/builds) unless explicitly allowed via env // CI override: set UNITY_MCP_ALLOW_BATCH=1 to allow the bridge in batch mode if (Application.isBatchMode && string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable("UNITY_MCP_ALLOW_BATCH"))) @@ -579,8 +621,32 @@ private static async Task HandleClientAsync(TcpClient client, CancellationToken { try { MCPForUnity.Editor.Helpers.McpLog.Info("[MCP] sending framed response", always: false); } catch { } } - byte[] responseBytes = System.Text.Encoding.UTF8.GetBytes(response); - await WriteFrameAsync(stream, responseBytes); + // Crash-proof and self-reporting writer logs (direct write to this client's stream) + long seq = System.Threading.Interlocked.Increment(ref _ioSeq); + byte[] responseBytes; + try + { + responseBytes = System.Text.Encoding.UTF8.GetBytes(response); + IoInfo($"[IO] โžœ write start seq={seq} tag=response len={responseBytes.Length} reqId=?"); + } + catch (Exception ex) + { + IoInfo($"[IO] โœ— serialize FAIL tag=response reqId=? {ex.GetType().Name}: {ex.Message}"); + throw; + } + + var swDirect = System.Diagnostics.Stopwatch.StartNew(); + try + { + await WriteFrameAsync(stream, responseBytes); + swDirect.Stop(); + IoInfo($"[IO] โœ“ write end tag=response len={responseBytes.Length} reqId=? durMs={swDirect.Elapsed.TotalMilliseconds:F1}"); + } + catch (Exception ex) + { + IoInfo($"[IO] โœ— write FAIL tag=response reqId=? {ex.GetType().Name}: {ex.Message}"); + throw; + } } catch (Exception ex) { diff --git a/UnityMcpBridge/Editor/Tools/ManageScene.cs b/UnityMcpBridge/Editor/Tools/ManageScene.cs index d3ae3d153..e68c19d7f 100644 --- a/UnityMcpBridge/Editor/Tools/ManageScene.cs +++ b/UnityMcpBridge/Editor/Tools/ManageScene.cs @@ -16,16 +16,46 @@ namespace MCPForUnity.Editor.Tools /// public static class ManageScene { + private sealed class SceneCommand + { + public string action { get; set; } = string.Empty; + public string name { get; set; } = string.Empty; + public string path { get; set; } = string.Empty; + public int? buildIndex { get; set; } + } + + private static SceneCommand ToSceneCommand(JObject p) + { + if (p == null) return new SceneCommand(); + int? BI(JToken t) + { + if (t == null || t.Type == JTokenType.Null) return null; + var s = t.ToString().Trim(); + if (s.Length == 0) return null; + if (int.TryParse(s, out var i)) return i; + if (double.TryParse(s, out var d)) return (int)d; + return t.Type == JTokenType.Integer ? t.Value() : (int?)null; + } + return new SceneCommand + { + action = (p["action"]?.ToString() ?? string.Empty).Trim().ToLowerInvariant(), + name = p["name"]?.ToString() ?? string.Empty, + path = p["path"]?.ToString() ?? string.Empty, + buildIndex = BI(p["buildIndex"] ?? p["build_index"]) + }; + } + /// /// Main handler for scene management actions. /// public static object HandleCommand(JObject @params) { try { McpLog.Info("[ManageScene] HandleCommand: start", always: false); } catch { } - string action = @params["action"]?.ToString().ToLower(); - string name = @params["name"]?.ToString(); - string path = @params["path"]?.ToString(); // Relative to Assets/ - int? buildIndex = @params["buildIndex"]?.ToObject(); + var cmd = ToSceneCommand(@params); + string action = cmd.action; + string name = string.IsNullOrEmpty(cmd.name) ? null : cmd.name; + string path = string.IsNullOrEmpty(cmd.path) ? null : cmd.path; // Relative to Assets/ + int? buildIndex = cmd.buildIndex; // bool loadAdditive = @params["loadAdditive"]?.ToObject() ?? false; // Example for future extension // Ensure path is relative to Assets/, removing any leading "Assets/" diff --git a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py index 67caa64b4..9435f0392 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py +++ b/UnityMcpBridge/UnityMcpServer~/src/tools/manage_scene.py @@ -14,9 +14,9 @@ def register_manage_scene_tools(mcp: FastMCP): def manage_scene( ctx: Context, action: str, - name: str, - path: str, - build_index: Any, + name: str = "", + path: str = "", + build_index: Any = None, ) -> Dict[str, Any]: """Manages Unity scenes (load, save, create, get hierarchy, etc.). @@ -47,15 +47,15 @@ def _coerce_int(value, default=None): except Exception: return default - build_index = _coerce_int(build_index, default=0) + coerced_build_index = _coerce_int(build_index, default=None) - params = { - "action": action, - "name": name, - "path": path, - "buildIndex": build_index - } - params = {k: v for k, v in params.items() if v is not None} + params = {"action": action} + if name: + params["name"] = name + if path: + params["path"] = path + if coerced_build_index is not None: + params["buildIndex"] = coerced_build_index # Use centralized retry helper response = send_command_with_retry("manage_scene", params) From 89714d022c35040e7861d209345e67beb84829de Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 10 Sep 2025 08:52:38 -0700 Subject: [PATCH 17/21] telemetry: record sub_action for tool executions; decorator extracts 'action'; add tests for keyword/positional extraction --- .../UnityMcpServer~/src/telemetry.py | 19 ++++- .../src/telemetry_decorator.py | 22 ++++- tests/test_telemetry_subaction.py | 83 +++++++++++++++++++ 3 files changed, 120 insertions(+), 4 deletions(-) create mode 100644 tests/test_telemetry_subaction.py diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 5e2031acf..413a7f9c7 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -356,13 +356,28 @@ def record_milestone(milestone: MilestoneType, data: Optional[Dict[str, Any]] = """Convenience function to record a milestone""" return get_telemetry().record_milestone(milestone, data) -def record_tool_usage(tool_name: str, success: bool, duration_ms: float, error: Optional[str] = None): - """Record tool usage telemetry""" +def record_tool_usage(tool_name: str, success: bool, duration_ms: float, error: Optional[str] = None, sub_action: Optional[str] = None): + """Record tool usage telemetry + + Args: + tool_name: Name of the tool invoked (e.g., 'manage_scene'). + success: Whether the tool completed successfully. + duration_ms: Execution duration in milliseconds. + error: Optional error message (truncated if present). + sub_action: Optional sub-action/operation within the tool (e.g., 'get_hierarchy'). + """ data = { "tool_name": tool_name, "success": success, "duration_ms": round(duration_ms, 2) } + + if sub_action is not None: + try: + data["sub_action"] = str(sub_action) + except Exception: + # Ensure telemetry is never disruptive + data["sub_action"] = "unknown" if error: data["error"] = str(error)[:200] # Limit error message length diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py index 875c66a41..3ea27d9e2 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py @@ -20,6 +20,15 @@ def _sync_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None + # Extract sub-action (e.g., 'get_hierarchy') from bound args when available + sub_action = None + try: + sig = inspect.signature(func) + bound = sig.bind_partial(*args, **kwargs) + bound.apply_defaults() + sub_action = bound.arguments.get("action") + except Exception: + sub_action = None try: global _decorator_log_count if _decorator_log_count < 10: @@ -38,13 +47,22 @@ def _sync_wrapper(*args, **kwargs) -> Any: raise finally: duration_ms = (time.time() - start_time) * 1000 - record_tool_usage(tool_name, success, duration_ms, error) + record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) @functools.wraps(func) async def _async_wrapper(*args, **kwargs) -> Any: start_time = time.time() success = False error = None + # Extract sub-action (e.g., 'get_hierarchy') from bound args when available + sub_action = None + try: + sig = inspect.signature(func) + bound = sig.bind_partial(*args, **kwargs) + bound.apply_defaults() + sub_action = bound.arguments.get("action") + except Exception: + sub_action = None try: global _decorator_log_count if _decorator_log_count < 10: @@ -63,7 +81,7 @@ async def _async_wrapper(*args, **kwargs) -> Any: raise finally: duration_ms = (time.time() - start_time) * 1000 - record_tool_usage(tool_name, success, duration_ms, error) + record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) return _async_wrapper if inspect.iscoroutinefunction(func) else _sync_wrapper return decorator \ No newline at end of file diff --git a/tests/test_telemetry_subaction.py b/tests/test_telemetry_subaction.py new file mode 100644 index 000000000..c1c597e28 --- /dev/null +++ b/tests/test_telemetry_subaction.py @@ -0,0 +1,83 @@ +import importlib + + +def _get_decorator_module(): + # Import the telemetry_decorator module from the Unity MCP server src + mod = importlib.import_module("UnityMcpBridge.UnityMcpServer~.src.telemetry_decorator") + return mod + + +def test_subaction_extracted_from_keyword(monkeypatch): + td = _get_decorator_module() + + captured = {} + + def fake_record_tool_usage(tool_name, success, duration_ms, error, sub_action=None): + captured["tool_name"] = tool_name + captured["success"] = success + captured["error"] = error + captured["sub_action"] = sub_action + + # Silence milestones/logging in test + monkeypatch.setattr(td, "record_tool_usage", fake_record_tool_usage) + monkeypatch.setattr(td, "record_milestone", lambda *a, **k: None) + monkeypatch.setattr(td, "_decorator_log_count", 999) + + def dummy_tool(ctx, action: str, name: str = ""): + return {"success": True, "name": name} + + wrapped = td.telemetry_tool("manage_scene")(dummy_tool) + + resp = wrapped(None, action="get_hierarchy", name="Sample") + assert resp["success"] is True + assert captured["tool_name"] == "manage_scene" + assert captured["success"] is True + assert captured["error"] is None + assert captured["sub_action"] == "get_hierarchy" + + +def test_subaction_extracted_from_positionals(monkeypatch): + td = _get_decorator_module() + + captured = {} + + def fake_record_tool_usage(tool_name, success, duration_ms, error, sub_action=None): + captured["tool_name"] = tool_name + captured["sub_action"] = sub_action + + monkeypatch.setattr(td, "record_tool_usage", fake_record_tool_usage) + monkeypatch.setattr(td, "record_milestone", lambda *a, **k: None) + monkeypatch.setattr(td, "_decorator_log_count", 999) + + def dummy_tool(ctx, action: str, name: str = ""): + return True + + wrapped = td.telemetry_tool("manage_scene")(dummy_tool) + + _ = wrapped(None, "save", "MyScene") + assert captured["tool_name"] == "manage_scene" + assert captured["sub_action"] == "save" + + +def test_subaction_none_when_not_present(monkeypatch): + td = _get_decorator_module() + + captured = {} + + def fake_record_tool_usage(tool_name, success, duration_ms, error, sub_action=None): + captured["tool_name"] = tool_name + captured["sub_action"] = sub_action + + monkeypatch.setattr(td, "record_tool_usage", fake_record_tool_usage) + monkeypatch.setattr(td, "record_milestone", lambda *a, **k: None) + monkeypatch.setattr(td, "_decorator_log_count", 999) + + def dummy_tool_without_action(ctx, name: str): + return 123 + + wrapped = td.telemetry_tool("apply_text_edits")(dummy_tool_without_action) + _ = wrapped(None, name="X") + assert captured["tool_name"] == "apply_text_edits" + assert captured["sub_action"] is None + + From 9f7308b4c2364deff17800cd37b0a69bf7bb4e64 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 10 Sep 2025 09:24:09 -0700 Subject: [PATCH 18/21] mcp-unity: telemetry fire-and-forget; safer sender reg; defer startup/conn telemetry; writer IO logs; manage_scene tolerant params; test worker wake --- .../Editor/Helpers/TelemetryHelper.cs | 10 +++- UnityMcpBridge/UnityMcpServer~/src/server.py | 59 +++++++++++-------- .../UnityMcpServer~/src/telemetry.py | 9 +-- tests/test_telemetry_queue_worker.py | 4 ++ 4 files changed, 52 insertions(+), 30 deletions(-) diff --git a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs index d15af82f0..4e068e990 100644 --- a/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs +++ b/UnityMcpBridge/Editor/Helpers/TelemetryHelper.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Threading; using UnityEngine; namespace MCPForUnity.Editor.Helpers @@ -124,7 +125,12 @@ public static void RecordEvent(string eventType, Dictionary data /// public static void RegisterTelemetrySender(Action> sender) { - s_sender = sender; + Interlocked.Exchange(ref s_sender, sender); + } + + public static void UnregisterTelemetrySender() + { + Interlocked.Exchange(ref s_sender, null); } /// @@ -179,7 +185,7 @@ public static void RecordToolExecution(string toolName, bool success, float dura private static void SendTelemetryToPythonServer(Dictionary telemetryData) { - var sender = s_sender; + var sender = Volatile.Read(ref s_sender); if (sender != null) { try diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index da5ae9487..24d1f9710 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -60,16 +60,18 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: server_version = ver_path.read_text(encoding="utf-8").strip() except Exception: server_version = "unknown" - # Defer telemetry for first second to avoid interfering with stdio handshake - if (time.perf_counter() - start_clk) > 1.0: - record_telemetry(RecordType.STARTUP, { - "server_version": server_version, - "startup_time": start_time - }) - - # Record first startup milestone - if (time.perf_counter() - start_clk) > 1.0: - record_milestone(MilestoneType.FIRST_STARTUP) + # Defer initial telemetry by 1s to avoid stdio handshake interference + import threading + def _emit_startup(): + try: + record_telemetry(RecordType.STARTUP, { + "server_version": server_version, + "startup_time": start_time, + }) + record_milestone(MilestoneType.FIRST_STARTUP) + except Exception: + logger.debug("Deferred startup telemetry failed", exc_info=True) + threading.Timer(1.0, _emit_startup).start() try: skip_connect = os.environ.get("UNITY_MCP_SKIP_STARTUP_CONNECT", "").lower() in ("1", "true", "yes", "on") @@ -79,33 +81,42 @@ async def server_lifespan(server: FastMCP) -> AsyncIterator[Dict[str, Any]]: _unity_connection = get_unity_connection() logger.info("Connected to Unity on startup") - # Record successful Unity connection - if (time.perf_counter() - start_clk) > 1.0: - record_telemetry(RecordType.UNITY_CONNECTION, { + # Record successful Unity connection (deferred) + import threading as _t + _t.Timer(1.0, lambda: record_telemetry( + RecordType.UNITY_CONNECTION, + { "status": "connected", - "connection_time_ms": (time.time() - start_time) * 1000 - }) + "connection_time_ms": (time.perf_counter() - start_clk) * 1000, + } + )).start() except ConnectionError as e: logger.warning("Could not connect to Unity on startup: %s", e) _unity_connection = None - # Record connection failure - if (time.perf_counter() - start_clk) > 1.0: - record_telemetry(RecordType.UNITY_CONNECTION, { + # Record connection failure (deferred) + import threading as _t + _t.Timer(1.0, lambda: record_telemetry( + RecordType.UNITY_CONNECTION, + { "status": "failed", "error": str(e)[:200], - "connection_time_ms": (time.perf_counter() - start_clk) * 1000 - }) + "connection_time_ms": (time.perf_counter() - start_clk) * 1000, + } + )).start() except Exception as e: logger.warning("Unexpected error connecting to Unity on startup: %s", e) _unity_connection = None - if (time.perf_counter() - start_clk) > 1.0: - record_telemetry(RecordType.UNITY_CONNECTION, { + import threading as _t + _t.Timer(1.0, lambda: record_telemetry( + RecordType.UNITY_CONNECTION, + { "status": "failed", "error": str(e)[:200], - "connection_time_ms": (time.perf_counter() - start_clk) * 1000 - }) + "connection_time_ms": (time.perf_counter() - start_clk) * 1000, + } + )).start() try: # Yield the connection object so it can be attached to the context diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 413a7f9c7..3de022ed9 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -169,9 +169,10 @@ def __init__(self): self._lock: threading.Lock = threading.Lock() # Bounded queue with single background worker (records only; no context propagation) self._queue: "queue.Queue[TelemetryRecord]" = queue.Queue(maxsize=1000) + # Load persistent data before starting worker so first events have UUID + self._load_persistent_data() self._worker: threading.Thread = threading.Thread(target=self._worker_loop, daemon=True) self._worker.start() - self._load_persistent_data() def _load_persistent_data(self): """Load UUID and milestones from disk""" @@ -307,8 +308,8 @@ def _send_telemetry(self, record: TelemetryRecord): # Re-validate endpoint at send time to handle dynamic changes endpoint = self.config._validated_endpoint(self.config.endpoint, self.config.default_endpoint) response = client.post(endpoint, json=payload) - if response.status_code == 200: - logger.info(f"Telemetry sent: {record.record_type}") + if 200 <= response.status_code < 300: + logger.debug(f"Telemetry sent: {record.record_type}") else: logger.warning(f"Telemetry failed: HTTP {response.status_code}") else: @@ -325,7 +326,7 @@ def _send_telemetry(self, record: TelemetryRecord): try: with urllib.request.urlopen(req, timeout=self.config.timeout) as resp: if 200 <= resp.getcode() < 300: - logger.info(f"Telemetry sent (urllib): {record.record_type}") + logger.debug(f"Telemetry sent (urllib): {record.record_type}") else: logger.warning(f"Telemetry failed (urllib): HTTP {resp.getcode()}") except urllib.error.URLError as ue: diff --git a/tests/test_telemetry_queue_worker.py b/tests/test_telemetry_queue_worker.py index c3e372279..09e4f90f4 100644 --- a/tests/test_telemetry_queue_worker.py +++ b/tests/test_telemetry_queue_worker.py @@ -45,9 +45,13 @@ def test_telemetry_queue_backpressure_and_single_worker(monkeypatch, caplog): # Force-enable telemetry regardless of env settings from conftest collector.config.enabled = True + # Wake existing worker once so it observes the new queue on the next loop + collector.record(telemetry.RecordType.TOOL_EXECUTION, {"i": -1}) # Replace queue with tiny one to trigger backpressure quickly small_q = q.Queue(maxsize=2) collector._queue = small_q + # Give the worker a moment to switch queues + time.sleep(0.02) # Make sends slow to build backlog and exercise worker def slow_send(self, rec): From 9b5488dcafabbde50f90f42f667e18b5ad4dac29 Mon Sep 17 00:00:00 2001 From: David Sarno Date: Wed, 10 Sep 2025 09:38:53 -0700 Subject: [PATCH 19/21] telemetry_decorator: guard record_tool_usage and milestone emits (sync/async) --- .../src/telemetry_decorator.py | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py index 3ea27d9e2..de94fb266 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry_decorator.py @@ -36,18 +36,25 @@ def _sync_wrapper(*args, **kwargs) -> Any: _decorator_log_count += 1 result = func(*args, **kwargs) success = True - if tool_name == "manage_script" and kwargs.get("action") == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) + action_val = sub_action or kwargs.get("action") + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: + _log.debug("milestone emit failed", exc_info=True) return result except Exception as e: error = str(e) raise finally: duration_ms = (time.time() - start_time) * 1000 - record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) + try: + record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) + except Exception: + _log.debug("record_tool_usage failed", exc_info=True) @functools.wraps(func) async def _async_wrapper(*args, **kwargs) -> Any: @@ -70,18 +77,25 @@ async def _async_wrapper(*args, **kwargs) -> Any: _decorator_log_count += 1 result = await func(*args, **kwargs) success = True - if tool_name == "manage_script" and kwargs.get("action") == "create": - record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) - elif tool_name.startswith("manage_scene"): - record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) - record_milestone(MilestoneType.FIRST_TOOL_USAGE) + action_val = sub_action or kwargs.get("action") + try: + if tool_name == "manage_script" and action_val == "create": + record_milestone(MilestoneType.FIRST_SCRIPT_CREATION) + elif tool_name.startswith("manage_scene"): + record_milestone(MilestoneType.FIRST_SCENE_MODIFICATION) + record_milestone(MilestoneType.FIRST_TOOL_USAGE) + except Exception: + _log.debug("milestone emit failed", exc_info=True) return result except Exception as e: error = str(e) raise finally: duration_ms = (time.time() - start_time) * 1000 - record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) + try: + record_tool_usage(tool_name, success, duration_ms, error, sub_action=sub_action) + except Exception: + _log.debug("record_tool_usage failed", exc_info=True) return _async_wrapper if inspect.iscoroutinefunction(func) else _sync_wrapper return decorator \ No newline at end of file From 33979d348b813b1bd8d8cd8c73baacd71b7d6631 Mon Sep 17 00:00:00 2001 From: dsarno Date: Wed, 10 Sep 2025 11:57:43 -0700 Subject: [PATCH 20/21] telemetry: use Cloud Run default, reject localhost overrides, add startup diagnostics, and normalize logging - config: point telemetry_endpoint to Cloud Run default - telemetry: log effective endpoint/timeout; reject localhost endpoints - server: telemetry logger at normal level with rotating file; default timeout=5s when unset --- UnityMcpBridge/UnityMcpServer~/src/config.py | 3 ++- UnityMcpBridge/UnityMcpServer~/src/server.py | 17 +++++++++++++++++ UnityMcpBridge/UnityMcpServer~/src/telemetry.py | 17 +++++++++++++++++ 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/config.py b/UnityMcpBridge/UnityMcpServer~/src/config.py index 5d0556725..f934ecebe 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/config.py +++ b/UnityMcpBridge/UnityMcpServer~/src/config.py @@ -36,7 +36,8 @@ class ServerConfig: # Telemetry settings telemetry_enabled: bool = True - telemetry_endpoint: str = "https://unity-mcp-telemetry-a6uvvbgbsa-uc.a.run.app/telemetry/events" + # Align with telemetry.py default Cloud Run endpoint + telemetry_endpoint: str = "https://unity-mcp-telemetry-375728817078.us-central1.run.app/telemetry/events" # Create a global config instance config = ServerConfig() \ No newline at end of file diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index 24d1f9710..af1d09912 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -29,6 +29,14 @@ _fh.setFormatter(logging.Formatter(config.log_format)) _fh.setLevel(getattr(logging, config.log_level)) logger.addHandler(_fh) + # Also route telemetry logger to the same rotating file and normal level + try: + tlog = logging.getLogger("unity-mcp-telemetry") + tlog.setLevel(getattr(logging, config.log_level)) + tlog.addHandler(_fh) + except Exception: + # Never let logging setup break startup + pass except Exception: # Never let logging setup break startup pass @@ -40,6 +48,15 @@ pass # Import telemetry only after logging is configured to ensure its logs use stderr and proper levels +# Ensure a slightly higher telemetry timeout unless explicitly overridden by env +try: + + + # Ensure generous timeout unless explicitly overridden by env + if not os.environ.get("UNITY_MCP_TELEMETRY_TIMEOUT"): + os.environ["UNITY_MCP_TELEMETRY_TIMEOUT"] = "5.0" +except Exception: + pass from telemetry import record_telemetry, record_milestone, RecordType, MilestoneType # Global connection state diff --git a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py index 3de022ed9..c679854e4 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/telemetry.py +++ b/UnityMcpBridge/UnityMcpServer~/src/telemetry.py @@ -96,6 +96,15 @@ def __init__(self): os.environ.get("UNITY_MCP_TELEMETRY_ENDPOINT", default_ep), default_ep, ) + try: + logger.info( + "Telemetry configured: endpoint=%s (default=%s), timeout_env=%s", + self.endpoint, + default_ep, + os.environ.get("UNITY_MCP_TELEMETRY_TIMEOUT") or "" + ) + except Exception: + pass # Local storage for UUID and milestones self.data_dir = self._get_data_directory() @@ -107,6 +116,10 @@ def __init__(self): self.timeout = float(os.environ.get("UNITY_MCP_TELEMETRY_TIMEOUT", "1.5")) except Exception: self.timeout = 1.5 + try: + logger.info("Telemetry timeout=%.2fs", self.timeout) + except Exception: + pass # Session tracking self.session_id = str(uuid.uuid4()) @@ -151,6 +164,10 @@ def _validated_endpoint(self, candidate: str, fallback: str) -> str: # Basic sanity: require network location and path if not parsed.netloc: raise ValueError("Missing netloc in endpoint") + # Reject localhost/loopback endpoints in production to avoid accidental local overrides + host = parsed.hostname or "" + if host in ("localhost", "127.0.0.1", "::1"): + raise ValueError("Localhost endpoints are not allowed for telemetry") return candidate except Exception as e: logger.debug( From 283597dc24070bd679374a4e78a533988a994abb Mon Sep 17 00:00:00 2001 From: dsarno Date: Wed, 10 Sep 2025 12:58:09 -0700 Subject: [PATCH 21/21] fix(startup): capture exception message before deferred telemetry lambda --- UnityMcpBridge/UnityMcpServer~/src/server.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/UnityMcpBridge/UnityMcpServer~/src/server.py b/UnityMcpBridge/UnityMcpServer~/src/server.py index af1d09912..b381a0ddc 100644 --- a/UnityMcpBridge/UnityMcpServer~/src/server.py +++ b/UnityMcpBridge/UnityMcpServer~/src/server.py @@ -114,11 +114,12 @@ def _emit_startup(): # Record connection failure (deferred) import threading as _t + _err_msg = str(e)[:200] _t.Timer(1.0, lambda: record_telemetry( RecordType.UNITY_CONNECTION, { "status": "failed", - "error": str(e)[:200], + "error": _err_msg, "connection_time_ms": (time.perf_counter() - start_clk) * 1000, } )).start() @@ -126,11 +127,12 @@ def _emit_startup(): logger.warning("Unexpected error connecting to Unity on startup: %s", e) _unity_connection = None import threading as _t + _err_msg = str(e)[:200] _t.Timer(1.0, lambda: record_telemetry( RecordType.UNITY_CONNECTION, { "status": "failed", - "error": str(e)[:200], + "error": _err_msg, "connection_time_ms": (time.perf_counter() - start_clk) * 1000, } )).start()