refactor: simplify telemetry - always enabled, build-time connection string #246
Conversation
…string - Remove opt-out functionality (EXCELMCP_TELEMETRY_OPTOUT) - Embed AppInsights connection string at build time via MSBuild - Add Directory.Build.props.user for local dev connection string - Remove runtime environment variable lookup for connection string - Update release workflows to pass connection string to build - Remove obsolete test.runsettings file - Simplify IsEnabled to always return true
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Reduce prompts from 14 to 2 (keep only high-value guides) - Remove 13 orphaned .md content files - Fix ExcelWorksheetTool incorrect 'save' action reference - Improve ExcelRangeTool and ExcelNamedRangeTool descriptions - Slim ServerInstructions to essential info only - Remove redundant telemetry flush (SDK handles automatically)
…lemetry configuration
- Set Component.Version explicitly to override SDK auto-detection (was picking up Excel COM interop 15.0.0.0 instead of app 1.0.0) - Anonymize cloud_RoleInstance (was exposing machine name) - Now all telemetry context values are privacy-safe: - application_Version: from assembly metadata - cloud_RoleName: fixed 'ExcelMcp.McpServer' - cloud_RoleInstance: anonymous hash 'instance-xxxx' - user_Id: anonymous hash of machine identity - session_Id: random per session
- Remove debug mode from telemetry (simplify code) - Fix PageView tracking to include duration (required for User Flows) - Always override application_Version (SDK picks up Excel 15.0.0.0) - Add explicit Flush() call in smoke tests for reliable telemetry delivery - Fix smoke test shutdown to use graceful shutdown before cancellation Telemetry now correctly populates: - requests table (with duration, success, resultCode) - pageViews table (with duration for User Flows) - user_Id and session_Id (for Users/Sessions blades) - application_Version (MCP Server version, not Excel)
| string.Equals(optOut, "1", StringComparison.OrdinalIgnoreCase); | ||
| // Connection string is embedded at build time from Directory.Build.props.user | ||
| // Returns null if not set (placeholder value starts with __) | ||
| if (string.IsNullOrEmpty(TelemetryConfig.ConnectionString) || |
Check warning
Code scanning / CodeQL
Constant condition Warning
Copilot Autofix
AI about 6 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| if (string.IsNullOrEmpty(TelemetryConfig.ConnectionString) || | ||
| TelemetryConfig.ConnectionString.StartsWith("__", StringComparison.Ordinal)) | ||
| { | ||
| Console.Error.WriteLine($"[TELEMETRY DIAG] GetConnectionString returning null. Raw value: '{TelemetryConfig.ConnectionString ?? "(null)"}'"); |
Check warning
Code scanning / CodeQL
Constant condition Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 6 hours ago
The best way to fix this is to simplify the logging line (line 79) so that it does not redundantly use the null-coalescing operator on TelemetryConfig.ConnectionString. Since the function returns early if TelemetryConfig.ConnectionString is either null or a placeholder, we can be certain it is not null at this log point, making the ?? "(null)" clause unnecessary.
Specifically, modify line 79 in src/ExcelMcp.McpServer/Telemetry/ExcelMcpTelemetry.cs to use TelemetryConfig.ConnectionString directly in the string interpolation, removing the unnecessary null check.
No imports or additional definitions are required for this change.
| @@ -76,7 +76,7 @@ | ||
| if (string.IsNullOrEmpty(TelemetryConfig.ConnectionString) || | ||
| TelemetryConfig.ConnectionString.StartsWith("__", StringComparison.Ordinal)) | ||
| { | ||
| Console.Error.WriteLine($"[TELEMETRY DIAG] GetConnectionString returning null. Raw value: '{TelemetryConfig.ConnectionString ?? "(null)"}'"); | ||
| Console.Error.WriteLine($"[TELEMETRY DIAG] GetConnectionString returning null. Raw value: '{TelemetryConfig.ConnectionString}'"); | ||
| return null; | ||
| } | ||
| Console.Error.WriteLine($"[TELEMETRY DIAG] GetConnectionString returning valid connection string (length={TelemetryConfig.ConnectionString.Length})"); |
Summary
Simplifies telemetry architecture by making it always enabled and embedding the AppInsights connection string at build time.
Changes
Telemetry Simplification
EXCELMCP_TELEMETRY_OPTOUTenvironment variable)IsEnabledto always returntrueIsOptedOut()methodBuild-Time Connection String
GenerateTelemetryConfigtargetWriteTextFileinline task to handle semicolons in connection stringsTelemetryConfig.g.cswith embedded connection stringLocal Development
Directory.Build.props.user.templatefor local dev setupDirectory.Build.props.user(gitignored) contains local connection stringAppInsightsConnectionStringproperty at build timeRelease Workflows
release-mcp-server.ymlto pass connection string via MSBuild propertyrelease-vscode-extension.ymlsimilarlyCleanup
test.runsettingsandtest.runsettings.localfilesRunSettingsFilePathfromtests/Directory.Build.propsdocs/DEVELOPMENT.mdTesting