Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions infra/aca.bicep
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ param openAiEndpoint string
param cosmosDbAccount string
param cosmosDbDatabase string
param cosmosDbContainer string
param applicationInsightsConnectionString string = ''

resource acaIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' = {
name: identityName
Expand Down Expand Up @@ -58,6 +59,11 @@ module app 'core/host/container-app-upsert.bicep' = {
name: 'AZURE_COSMOSDB_CONTAINER'
value: cosmosDbContainer
}
// We typically store sensitive values in secrets, but App Insights connection strings are not considered highly sensitive
{
name: 'APPLICATIONINSIGHTS_CONNECTION_STRING'
value: applicationInsightsConnectionString
}
Comment on lines +63 to +66
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

When Application Insights connection string is empty (when useMonitoring is false), the environment variable APPLICATIONINSIGHTS_CONNECTION_STRING will be set to an empty string in the container app. This could potentially trigger the Azure Monitor instrumentation setup in deployed_mcp.py (line 35-37) with an invalid connection string, leading to errors. Consider either not adding the environment variable when it's empty, or checking for non-empty values in the Python code before calling configure_azure_monitor().

Suggested change
{
name: 'APPLICATIONINSIGHTS_CONNECTION_STRING'
value: applicationInsightsConnectionString
}
// Only add the environment variable if the connection string is non-empty
...(empty(applicationInsightsConnectionString) ? [] : [
{
name: 'APPLICATIONINSIGHTS_CONNECTION_STRING'
value: applicationInsightsConnectionString
}
])

Copilot uses AI. Check for mistakes.
Comment on lines +65 to +66
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

APPLICATIONINSIGHTS_CONNECTION_STRING is set as a plain environment variable for the container app, increasing chances of accidental exposure via logs, crash dumps, or diagnostics endpoints. Attackers with limited foothold (e.g., read access to env or logs) could exfiltrate it and push malicious telemetry. Fix: store this value as a secret (Container Apps secrets or Key Vault) and reference it from the app; avoid placing it directly in environmentVariables.

Copilot uses AI. Check for mistakes.
]
targetPort: 8000
}
Expand Down
18 changes: 18 additions & 0 deletions infra/main.bicep
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,20 @@ module logAnalyticsWorkspace 'br/public:avm/res/operational-insights/workspace:0
}
}

// Application Insights for telemetry
module applicationInsights 'br/public:avm/res/insights/component:0.4.2' = if (useMonitoring) {
name: 'applicationinsights'
scope: resourceGroup
params: {
name: '${prefix}-appinsights'
location: location
tags: tags
workspaceResourceId: logAnalyticsWorkspace.?outputs.resourceId
kind: 'web'
applicationType: 'web'
}
}

// https://learn.microsoft.com/en-us/azure/container-apps/firewall-integration?tabs=consumption-only
module containerAppsNSG 'br/public:avm/res/network/network-security-group:0.5.1' = if (useVnet) {
name: 'containerAppsNSG'
Expand Down Expand Up @@ -669,6 +683,7 @@ module aca 'aca.bicep' = {
cosmosDbAccount: cosmosDb.outputs.name
cosmosDbDatabase: cosmosDbDatabaseName
cosmosDbContainer: cosmosDbContainerName
applicationInsightsConnectionString: useMonitoring ? applicationInsights.outputs.connectionString : ''
exists: acaExists
}
}
Expand Down Expand Up @@ -738,3 +753,6 @@ output AZURE_COSMOSDB_ACCOUNT string = cosmosDb.outputs.name
output AZURE_COSMOSDB_ENDPOINT string = cosmosDb.outputs.endpoint
output AZURE_COSMOSDB_DATABASE string = cosmosDbDatabaseName
output AZURE_COSMOSDB_CONTAINER string = cosmosDbContainerName

// We typically do not output sensitive values, but App Insights connection strings are not considered highly sensitive
output APPLICATIONINSIGHTS_CONNECTION_STRING string = useMonitoring ? applicationInsights.outputs.connectionString : ''
Comment on lines +757 to +758
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

APPLICATIONINSIGHTS_CONNECTION_STRING is being exported as a top-level output, which can be viewed by anyone with access to deployment outputs and may be logged or surfaced in tooling. This connection string includes the instrumentation key and can be used to submit telemetry to your Application Insights resource, making it sensitive. Mitigation: avoid outputting this value or mark it as a secure output; instead fetch it securely at runtime or store it in a secret store (e.g., Key Vault) and reference it from deployments.

Suggested change
// We typically do not output sensitive values, but App Insights connection strings are not considered highly sensitive
output APPLICATIONINSIGHTS_CONNECTION_STRING string = useMonitoring ? applicationInsights.outputs.connectionString : ''
// Application Insights connection string is sensitive and should not be output as a deployment output.

Copilot uses AI. Check for mistakes.
1 change: 1 addition & 0 deletions infra/write_env.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,5 @@ Add-Content -Path $ENV_FILE_PATH -Value "AZURE_TENANT_ID=$(azd env get-value AZU
Add-Content -Path $ENV_FILE_PATH -Value "AZURE_COSMOSDB_ACCOUNT=$(azd env get-value AZURE_COSMOSDB_ACCOUNT)"
Add-Content -Path $ENV_FILE_PATH -Value "AZURE_COSMOSDB_DATABASE=$(azd env get-value AZURE_COSMOSDB_DATABASE)"
Add-Content -Path $ENV_FILE_PATH -Value "AZURE_COSMOSDB_CONTAINER=$(azd env get-value AZURE_COSMOSDB_CONTAINER)"
Add-Content -Path $ENV_FILE_PATH -Value "APPLICATIONINSIGHTS_CONNECTION_STRING=$(azd env get-value APPLICATIONINSIGHTS_CONNECTION_STRING)"
Add-Content -Path $ENV_FILE_PATH -Value "API_HOST=azure"
3 changes: 2 additions & 1 deletion infra/write_env.sh
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ echo "AZURE_TENANT_ID=$(azd env get-value AZURE_TENANT_ID)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_ACCOUNT=$(azd env get-value AZURE_COSMOSDB_ACCOUNT)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_DATABASE=$(azd env get-value AZURE_COSMOSDB_DATABASE)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_CONTAINER=$(azd env get-value AZURE_COSMOSDB_CONTAINER)" >> "$ENV_FILE_PATH"
echo "API_HOST=azure" >> "$ENV_FILE_PATH"
echo "APPLICATIONINSIGHTS_CONNECTION_STRING=$(azd env get-value APPLICATIONINSIGHTS_CONNECTION_STRING)" >> "$ENV_FILE_PATH"
echo "API_HOST=azure" >> "$ENV_FILE_PATH"
4 changes: 4 additions & 0 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ dependencies = [
"azure-ai-agents>=1.1.0",
"agent-framework>=1.0.0b251016",
"azure-cosmos>=4.9.0",
"azure-monitor-opentelemetry>=1.6.4",
"opentelemetry-instrumentation-starlette>=0.49b0",
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The version constraint >=0.49b0 for opentelemetry-instrumentation-starlette doesn't match the actual version in uv.lock which is 0.59b0. Consider updating the constraint to >=0.59b0 to match the locked version and ensure compatibility, or use a more specific constraint like >=0.59b0,<0.60 to prevent unexpected breaking changes from beta versions.

Suggested change
"opentelemetry-instrumentation-starlette>=0.49b0",
"opentelemetry-instrumentation-starlette>=0.59b0,<0.60",

Copilot uses AI. Check for mistakes.
"logfire>=4.15.1",
"azure-core-tracing-opentelemetry>=1.0.0b12"
]

[dependency-groups]
Expand Down
Empty file added servers/__init__.py
Empty file.
36 changes: 29 additions & 7 deletions servers/deployed_mcp.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,49 @@
"""Run with: cd servers && uvicorn deployed_mcp:app --host 0.0.0.0 --port 8000"""

import logging
import os
import uuid
from datetime import date
from enum import Enum
from typing import Annotated

import logfire
from azure.core.settings import settings
from azure.cosmos.aio import CosmosClient
from azure.identity.aio import DefaultAzureCredential, ManagedIdentityCredential
from azure.monitor.opentelemetry import configure_azure_monitor
from dotenv import load_dotenv
from fastmcp import FastMCP
from opentelemetry.instrumentation.starlette import StarletteInstrumentor

try:
from opentelemetry_middleware import OpenTelemetryMiddleware
except ImportError:
from servers.opentelemetry_middleware import OpenTelemetryMiddleware

RUNNING_IN_PRODUCTION = os.getenv("RUNNING_IN_PRODUCTION", "false").lower() == "true"

load_dotenv(override=True)
if not RUNNING_IN_PRODUCTION:
load_dotenv(override=True)

logging.basicConfig(level=logging.INFO, format="%(asctime)s - %(message)s")
logging.basicConfig(level=logging.WARNING, format="%(asctime)s - %(message)s")
logger = logging.getLogger("ExpensesMCP")
logger.setLevel(logging.INFO)

# Configure OpenTelemetry tracing, either via Azure Monitor or Logfire
# We don't support both at the same time due to potential conflicts with tracer providers
if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"):
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The conditional logic checks for the presence of environment variables using os.getenv() which returns None for unset variables but could also return empty strings. When useMonitoring is false in the Bicep deployment, APPLICATIONINSIGHTS_CONNECTION_STRING will be set to an empty string (see aca.bicep line 66), which would pass this truthy check and call configure_azure_monitor() with an invalid connection string. Update the condition to check for non-empty strings: if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"): should be if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING") and os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING").strip():

Suggested change
if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"):
connection_string = os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING")
if connection_string and connection_string.strip():

Copilot uses AI. Check for mistakes.
logger.info("Setting up Azure Monitor instrumentation")
configure_azure_monitor()
elif os.getenv("LOGFIRE_PROJECT_NAME"):
logger.info("Setting up Logfire instrumentation")
settings.tracing_implementation = "opentelemetry" # Configure Azure SDK to use OpenTelemetry tracing
Comment on lines +35 to +40
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The Azure SDK tracing implementation setting (settings.tracing_implementation = "opentelemetry") is only configured when using Logfire, but not when using Azure Monitor. This could lead to inconsistent Azure SDK tracing behavior between the two observability platforms. Consider setting this configuration before the conditional blocks (line 33) to ensure Azure SDK always uses OpenTelemetry tracing regardless of the export destination.

Suggested change
if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"):
logger.info("Setting up Azure Monitor instrumentation")
configure_azure_monitor()
elif os.getenv("LOGFIRE_PROJECT_NAME"):
logger.info("Setting up Logfire instrumentation")
settings.tracing_implementation = "opentelemetry" # Configure Azure SDK to use OpenTelemetry tracing
settings.tracing_implementation = "opentelemetry" # Ensure Azure SDK always uses OpenTelemetry tracing
if os.getenv("APPLICATIONINSIGHTS_CONNECTION_STRING"):
logger.info("Setting up Azure Monitor instrumentation")
configure_azure_monitor()
elif os.getenv("LOGFIRE_PROJECT_NAME"):
logger.info("Setting up Logfire instrumentation")

Copilot uses AI. Check for mistakes.
logfire.configure(service_name="expenses-mcp", send_to_logfire=True)
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

The send_to_logfire=True parameter is redundant when LOGFIRE_PROJECT_NAME is set. The logfire.configure() function automatically enables sending to Logfire when a project name is configured. This parameter should be omitted for clarity.

Suggested change
logfire.configure(service_name="expenses-mcp", send_to_logfire=True)
logfire.configure(service_name="expenses-mcp")

Copilot uses AI. Check for mistakes.

# Cosmos DB configuration from environment variables
AZURE_COSMOSDB_ACCOUNT = os.environ["AZURE_COSMOSDB_ACCOUNT"]
AZURE_COSMOSDB_DATABASE = os.environ["AZURE_COSMOSDB_DATABASE"]
AZURE_COSMOSDB_CONTAINER = os.environ["AZURE_COSMOSDB_CONTAINER"]
RUNNING_IN_PRODUCTION = os.getenv("RUNNING_IN_PRODUCTION", "false").lower() == "true"
AZURE_CLIENT_ID = os.getenv("AZURE_CLIENT_ID", "")

# Configure Cosmos DB client and container
Expand All @@ -37,6 +61,7 @@
logger.info(f"Connected to Cosmos DB: {AZURE_COSMOSDB_ACCOUNT}")

mcp = FastMCP("Expenses Tracker")
mcp.add_middleware(OpenTelemetryMiddleware("ExpensesMCP"))


class PaymentMethod(Enum):
Expand Down Expand Up @@ -152,7 +177,4 @@ def analyze_spending_prompt(

# ASGI application for uvicorn
app = mcp.http_app()

if __name__ == "__main__":
logger.info("MCP Expenses server starting (HTTP mode on port 8000)")
mcp.run(transport="http", host="0.0.0.0", port=8000)
StarletteInstrumentor.instrument_app(app)
85 changes: 85 additions & 0 deletions servers/opentelemetry_middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
from fastmcp.server.middleware import Middleware, MiddlewareContext
from opentelemetry import trace
from opentelemetry.trace import Status, StatusCode


class OpenTelemetryMiddleware(Middleware):
"""Middleware that creates OpenTelemetry spans for MCP operations."""

def __init__(self, tracer_name: str):
self.tracer = trace.get_tracer(tracer_name)

async def on_call_tool(self, context: MiddlewareContext, call_next):
"""Create a span for each tool call with detailed attributes."""
tool_name = context.message.name

with self.tracer.start_as_current_span(
f"tool.{tool_name}",
attributes={
"mcp.method": context.method,
"mcp.source": context.source,
"mcp.tool.name": tool_name,
# If arguments are sensitive, consider omitting or sanitizing them
# If arguments are long/nested, consider adding a size or depth limit
"mcp.tool.arguments": str(context.message.arguments),
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

Potential security concern: Tool arguments are logged to OpenTelemetry spans as-is with str(context.message.arguments). This could expose sensitive data (e.g., amounts, descriptions, personal information) to the observability platform. Consider implementing a sanitization strategy or providing configuration to exclude sensitive fields from telemetry data. The comment on line 22-23 acknowledges this but doesn't implement a solution.

Copilot uses AI. Check for mistakes.
},
) as span:
try:
result = await call_next(context)
span.set_attribute("mcp.tool.success", True)
span.set_status(Status(StatusCode.OK))
return result
except Exception as e:
span.set_attribute("mcp.tool.success", False)
span.set_attribute("mcp.tool.error", str(e))
span.set_status(Status(StatusCode.ERROR, str(e)))
span.record_exception(e)
raise

async def on_read_resource(self, context: MiddlewareContext, call_next):
"""Create a span for each resource read."""
resource_uri = str(getattr(context.message, "uri", "unknown"))

with self.tracer.start_as_current_span(
f"resource.{resource_uri}",
attributes={
"mcp.method": context.method,
"mcp.source": context.source,
"mcp.resource.uri": resource_uri,
},
) as span:
try:
result = await call_next(context)
span.set_attribute("mcp.resource.success", True)
span.set_status(Status(StatusCode.OK))
return result
except Exception as e:
span.set_attribute("mcp.resource.success", False)
span.set_attribute("mcp.resource.error", str(e))
span.set_status(Status(StatusCode.ERROR, str(e)))
span.record_exception(e)
raise

async def on_get_prompt(self, context: MiddlewareContext, call_next):
"""Create a span for each prompt retrieval."""
prompt_name = getattr(context.message, "name", "unknown")

with self.tracer.start_as_current_span(
f"prompt.{prompt_name}",
Comment on lines +17 to +68
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

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

[nitpick] The span naming pattern f"tool.{tool_name}", f"resource.{resource_uri}", and f"prompt.{prompt_name}" could lead to high cardinality issues if this middleware is reused in contexts where these identifiers contain dynamic values (e.g., UUIDs, timestamps). For better observability practices, consider using fixed span names (e.g., "tool.call", "resource.read", "prompt.get") with the specific identifier stored only as an attribute, which is already being done.

Copilot uses AI. Check for mistakes.
attributes={
"mcp.method": context.method,
"mcp.source": context.source,
"mcp.prompt.name": prompt_name,
},
) as span:
try:
result = await call_next(context)
span.set_attribute("mcp.prompt.success", True)
span.set_status(Status(StatusCode.OK))
return result
except Exception as e:
span.set_attribute("mcp.prompt.success", False)
span.set_attribute("mcp.prompt.error", str(e))
span.set_status(Status(StatusCode.ERROR, str(e)))
span.record_exception(e)
raise
69 changes: 69 additions & 0 deletions uv.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.