Skip to content
This repository was archived by the owner on Sep 4, 2025. It is now read-only.

Conversation

@conniey
Copy link
Member

@conniey conniey commented Jul 25, 2025

What does this PR do?

  • Remove unnecessary null-forgiving operator uses. (From the API these values are never expected to be null.)
  • Remove extra invocation of ToolExecuted activity.
  • Broaden MAC Address search to limit null values.

GitHub issue number?

Pre-merge Checklist

  • Required for All PRs
    • Read contribution guidelines
    • PR title clearly describes the change
    • Commit history is clean with descriptive messages (cleanup guide)
    • Added comprehensive tests for new/modified functionality
    • Updated CHANGELOG.md for product changes (features, bug fixes, UI/UX, updated dependencies)
    • Spelling check passes: .\eng\common\spelling\Invoke-Cspell.ps1
  • For MCP tool changes, updated:
    • Updated README.md documentation
    • Updated command list in /docs/azmcp-commands.md
    • Updated test prompts in /e2eTests/e2eTestPrompts.md
  • 👉 For Community (non-Azure team member) PRs:
    • Security review: Reviewed code for security vulnerabilities, malicious code, or suspicious activities before running tests (crypto mining, spam, data exfiltration, etc.)
    • Manual tests run: added comment /azp run azure - mcp to run Live Test Pipeline

Copilot AI review requested due to automatic review settings July 25, 2025 18:43
@conniey conniey requested a review from a team as a code owner July 25, 2025 18:43
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes telemetry issues by removing duplicate activity tracking and improving MAC address collection for telemetry purposes.

Key changes:

  • Removed redundant ToolExecuted activity invocation in McpRuntime
  • Enhanced MAC address collection to exclude loopback interfaces and handle null values more robustly

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
core/src/AzureMcp.Core/Extensions/OpenTelemetryExtensions.cs Improved MAC address collection by filtering out loopback interfaces and providing better null handling
core/src/AzureMcp.Core/Areas/Server/Commands/Runtime/McpRuntime.cs Removed duplicate telemetry activity tracking and simplified CallToolHandler method

Copy link
Member

@anuchandy anuchandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, ty Connie!

@github-project-automation github-project-automation bot moved this from Untriaged to In Progress in Azure MCP Server (OLD) Jul 25, 2025
@conniey conniey mentioned this pull request Jul 28, 2025
14 tasks
@conniey conniey force-pushed the update-telemetry-properties branch from 3350c3d to 2f9c97c Compare August 6, 2025 20:39
Copy link
Member

@anuchandy anuchandy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ty Connie! LGTM

@conniey conniey changed the title Telemetry fixes Telemetry fixes for ToolName and passing context. Aug 6, 2025
@conniey conniey merged commit 3781a2b into Azure:main Aug 6, 2025
27 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in Azure MCP Server (OLD) Aug 6, 2025
@conniey conniey deleted the update-telemetry-properties branch August 6, 2025 22:14
@wchigit wchigit mentioned this pull request Aug 8, 2025
13 tasks
chidozieononiwu pushed a commit to chidozieononiwu/azure-mcp that referenced this pull request Aug 18, 2025
* Add documentation about handling errors that occur in a loader.

* Remove Activity handling inside CommandFactoryToolLoader.

* Update McpRuntime to set the Activity status depending on result from calling a tool.

* Fix constructor change

* Add tests for telemetry.

* Update CHANGELOG

* Add tests for Tagging ToolName

* Pass in command context of current activity if available.

* Add current context to cli
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants