From aaedf01a3ea2dfd9c9293772543b96996b2b6239 Mon Sep 17 00:00:00 2001 From: Michael He Date: Wed, 8 Oct 2025 19:51:56 +0000 Subject: [PATCH] fix(cloudwatch-appsignals-mcp-server): update get_service_detail tool parameters --- .../service_tools.py | 117 +++-- .../tests/test_server.py | 460 +++++++++++++++--- .../tests/test_service_tools_operations.py | 306 ++++++------ 3 files changed, 589 insertions(+), 294 deletions(-) diff --git a/src/cloudwatch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/service_tools.py b/src/cloudwatch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/service_tools.py index 05eade6c2c..9c5d528883 100644 --- a/src/cloudwatch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/service_tools.py +++ b/src/cloudwatch-appsignals-mcp-server/awslabs/cloudwatch_appsignals_mcp_server/service_tools.py @@ -124,7 +124,16 @@ async def list_monitored_services() -> str: async def get_service_detail( service_name: str = Field( - ..., description='Name of the service to get details for (case-sensitive)' + ..., + description='Name of the service to get details for (case-sensitive). Use list_monitored_services() to see available services.', + ), + environment: str = Field( + ..., + description='Environment of the service (e.g., production, staging, dev). Use list_monitored_services() to see available environments.', + ), + service_type: str = Field( + default='Service', + description='Type of the service. Defaults to "Service" for standard Application Signals services. Other types: RemoteService, AWS::Service. Use list_monitored_services() to see the Type for each service.', ), ) -> str: """Get detailed information about a specific Application Signals service. @@ -160,34 +169,24 @@ async def get_service_detail( but audit_services() is the primary tool for operation discovery and performance analysis. """ start_time_perf = timer() - logger.debug(f'Starting get_service_healthy_detail request for service: {service_name}') + logger.debug( + f'Starting get_service_healthy_detail request for service: {service_name}, environment: {environment}, type: {service_type}' + ) try: # Calculate time range (last 24 hours) end_time = datetime.now(timezone.utc) start_time = end_time - timedelta(hours=24) - # First, get all services to find the one we want - services_response = appsignals_client.list_services( - StartTime=start_time, EndTime=end_time, MaxResults=100 - ) - - # Find the service with matching name - target_service = None - for service in services_response.get('ServiceSummaries', []): - key_attrs = service.get('KeyAttributes', {}) - if key_attrs.get('Name') == service_name: - target_service = service - break - - if not target_service: - logger.warning(f"Service '{service_name}' not found in Application Signals") - return f"Service '{service_name}' not found in Application Signals." + # Build KeyAttributes from parameters + key_attributes = {'Type': service_type, 'Name': service_name, 'Environment': environment} # Get detailed service information - logger.debug(f'Getting detailed information for service: {service_name}') + logger.debug( + f'Getting detailed information for service: {service_name} in environment: {environment}' + ) service_response = appsignals_client.get_service( - StartTime=start_time, EndTime=end_time, KeyAttributes=target_service['KeyAttributes'] + StartTime=start_time, EndTime=end_time, KeyAttributes=key_attributes ) service_details = service_response['Service'] @@ -243,7 +242,7 @@ async def get_service_detail( error_code = e.response.get('Error', {}).get('Code', 'Unknown') error_message = e.response.get('Error', {}).get('Message', 'Unknown error') logger.error(f'AWS ClientError in get_service_detail: {error_code} - {error_message}') - return f'AWS Error: {error_message}' + return f"AWS Error: {error_message}\n\nService not found with Name='{service_name}', Environment='{environment}', Type='{service_type}'. Use list_monitored_services() to see available services." except Exception as e: logger.error( f"Unexpected error in get_service_healthy_detail for '{service_name}': {str(e)}", @@ -254,12 +253,21 @@ async def get_service_detail( async def query_service_metrics( service_name: str = Field( - ..., description='Name of the service to get metrics for (case-sensitive)' + ..., + description='Name of the service to get metrics for (case-sensitive). Use list_monitored_services() to see available services.', + ), + environment: str = Field( + ..., + description='Environment of the service (e.g., production, staging, dev). Use list_monitored_services() to see available environments.', ), metric_name: str = Field( ..., description='Specific metric name (e.g., Latency, Error, Fault). Leave empty to list available metrics', ), + service_type: str = Field( + default='Service', + description='Type of the service. Defaults to "Service" for standard Application Signals services. Other types: RemoteService, AWS::Service. Use list_monitored_services() to see the Type for each service.', + ), statistic: str = Field( default='Average', description='Standard statistic type (Average, Sum, Maximum, Minimum, SampleCount)', @@ -296,7 +304,7 @@ async def query_service_metrics( """ start_time_perf = timer() logger.info( - f'Starting query_service_metrics request - service: {service_name}, metric: {metric_name}, hours: {hours}' + f'Starting query_service_metrics request - service: {service_name}, environment: {environment}, metric: {metric_name}, hours: {hours}' ) try: @@ -304,26 +312,12 @@ async def query_service_metrics( end_time = datetime.now(timezone.utc) start_time = end_time - timedelta(hours=hours) - # Get service details to find metrics - services_response = appsignals_client.list_services( - StartTime=start_time, EndTime=end_time, MaxResults=100 - ) - - # Find the target service - target_service = None - for service in services_response.get('ServiceSummaries', []): - key_attrs = service.get('KeyAttributes', {}) - if key_attrs.get('Name') == service_name: - target_service = service - break - - if not target_service: - logger.warning(f"Service '{service_name}' not found in Application Signals") - return f"Service '{service_name}' not found in Application Signals." + # Build KeyAttributes from parameters + key_attributes = {'Type': service_type, 'Name': service_name, 'Environment': environment} # Get detailed service info for metric references service_response = appsignals_client.get_service( - StartTime=start_time, EndTime=end_time, KeyAttributes=target_service['KeyAttributes'] + StartTime=start_time, EndTime=end_time, KeyAttributes=key_attributes ) metric_refs = service_response['Service'].get('MetricReferences', []) @@ -449,7 +443,7 @@ async def query_service_metrics( error_code = e.response.get('Error', {}).get('Code', 'Unknown') error_message = e.response.get('Error', {}).get('Message', 'Unknown error') logger.error(f'AWS ClientError in query_service_metrics: {error_code} - {error_message}') - return f'AWS Error: {error_message}' + return f"AWS Error: {error_message}\n\nService not found with Name='{service_name}', Environment='{environment}', Type='{service_type}'. Use list_monitored_services() to see available services." except Exception as e: logger.error( f"Unexpected error in query_service_metrics for '{service_name}/{metric_name}': {str(e)}", @@ -460,7 +454,16 @@ async def query_service_metrics( async def list_service_operations( service_name: str = Field( - ..., description='Name of the service to list operations for (case-sensitive)' + ..., + description='Name of the service to list operations for (case-sensitive). Use list_monitored_services() to see available services.', + ), + environment: str = Field( + ..., + description='Environment of the service (e.g., production, staging, dev). Use list_monitored_services() to see available environments.', + ), + service_type: str = Field( + default='Service', + description='Type of the service. Defaults to "Service" for standard Application Signals services. Other types: RemoteService, AWS::Service. Use list_monitored_services() to see the Type for each service.', ), hours: int = Field( default=24, @@ -516,7 +519,9 @@ async def list_service_operations( comprehensive operation auditing, performance analysis, and operation insights regardless of recent activity. """ start_time_perf = timer() - logger.debug(f'Starting list_service_operations request for service: {service_name}') + logger.debug( + f'Starting list_service_operations request for service: {service_name}, environment: {environment}' + ) try: # Calculate time range - enforce 24 hour maximum for Application Signals operation discovery @@ -524,29 +529,17 @@ async def list_service_operations( hours = min(hours, 24) # Enforce maximum of 24 hours start_time = end_time - timedelta(hours=hours) - # First, get the service to find its key attributes - services_response = appsignals_client.list_services( - StartTime=start_time, EndTime=end_time, MaxResults=100 - ) - - # Find the target service - target_service = None - for service in services_response.get('ServiceSummaries', []): - key_attrs = service.get('KeyAttributes', {}) - if key_attrs.get('Name') == service_name: - target_service = service - break - - if not target_service: - logger.warning(f"Service '{service_name}' not found in Application Signals") - return f"Service '{service_name}' not found in Application Signals. Use list_monitored_services() to see available services." + # Build KeyAttributes from parameters + key_attributes = {'Type': service_type, 'Name': service_name, 'Environment': environment} # Get operations for the service using ListServiceOperations API - logger.debug(f'Getting operations for service: {service_name}') + logger.debug( + f'Getting operations for service: {service_name} in environment: {environment}' + ) operations_response = appsignals_client.list_service_operations( StartTime=start_time, EndTime=end_time, - KeyAttributes=target_service['KeyAttributes'], + KeyAttributes=key_attributes, MaxResults=100, ) @@ -650,7 +643,7 @@ async def list_service_operations( error_code = e.response.get('Error', {}).get('Code', 'Unknown') error_message = e.response.get('Error', {}).get('Message', 'Unknown error') logger.error(f'AWS ClientError in list_service_operations: {error_code} - {error_message}') - return f'AWS Error: {error_message}' + return f"AWS Error: {error_message}\n\nService not found with Name='{service_name}', Environment='{environment}', Type='{service_type}'. Use list_monitored_services() to see available services." except Exception as e: logger.error( f"Unexpected error in list_service_operations for '{service_name}': {str(e)}", diff --git a/src/cloudwatch-appsignals-mcp-server/tests/test_server.py b/src/cloudwatch-appsignals-mcp-server/tests/test_server.py index 22470fe5a1..35feabd016 100644 --- a/src/cloudwatch-appsignals-mcp-server/tests/test_server.py +++ b/src/cloudwatch-appsignals-mcp-server/tests/test_server.py @@ -147,15 +147,13 @@ async def test_list_monitored_services_empty(mock_aws_clients): @pytest.mark.asyncio async def test_get_service_detail_success(mock_aws_clients): """Test successful retrieval of service details.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = { 'Service': { - 'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}, + 'KeyAttributes': { + 'Name': 'test-service', + 'Type': 'Service', + 'Environment': 'production', + }, 'AttributeMaps': [{'Platform': 'ECS', 'Application': 'test-app'}], 'MetricReferences': [ { @@ -169,13 +167,13 @@ async def test_get_service_detail_success(mock_aws_clients): } } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response - result = await get_service_detail('test-service') + result = await get_service_detail(service_name='test-service', environment='production') assert 'Service Details: test-service' in result - assert 'AWS::ECS::Service' in result + assert 'Type: Service' in result + assert 'Environment: production' in result assert 'Platform: ECS' in result assert 'AWS/ApplicationSignals/Latency' in result assert '/aws/ecs/test-service' in result @@ -184,24 +182,232 @@ async def test_get_service_detail_success(mock_aws_clients): @pytest.mark.asyncio async def test_get_service_detail_not_found(mock_aws_clients): """Test when service is not found.""" - mock_response = {'ServiceSummaries': []} + mock_aws_clients['appsignals_client'].get_service.side_effect = ClientError( + error_response={ + 'Error': { + 'Code': 'ResourceNotFoundException', + 'Message': 'Service not found', + } + }, + operation_name='GetService', + ) - mock_aws_clients['appsignals_client'].list_services.return_value = mock_response + result = await get_service_detail(service_name='nonexistent-service', environment='production') + + assert 'AWS Error: Service not found' in result + assert "Service not found with Name='nonexistent-service', Environment='production'" in result + + +@pytest.mark.asyncio +async def test_get_service_detail_distinguishes_environments(mock_aws_clients): + """Test that we can distinguish between same service name in different environments.""" + # Mock response for production environment + mock_production_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'api-service', + 'Type': 'Service', + 'Environment': 'production', + }, + 'AttributeMaps': [{'Platform': 'EKS', 'Application': 'prod-app'}], + } + } + + # Mock response for staging environment + mock_staging_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'api-service', + 'Type': 'Service', + 'Environment': 'staging', + }, + 'AttributeMaps': [{'Platform': 'ECS', 'Application': 'staging-app'}], + } + } + + # Test production environment + mock_aws_clients['appsignals_client'].get_service.return_value = mock_production_response + result_prod = await get_service_detail(service_name='api-service', environment='production') + + assert 'Service Details: api-service' in result_prod + assert 'Environment: production' in result_prod + assert 'Platform: EKS' in result_prod + assert 'Application: prod-app' in result_prod - result = await get_service_detail('nonexistent-service') + # Test staging environment + mock_aws_clients['appsignals_client'].get_service.return_value = mock_staging_response + result_staging = await get_service_detail(service_name='api-service', environment='staging') - assert "Service 'nonexistent-service' not found" in result + assert 'Service Details: api-service' in result_staging + assert 'Environment: staging' in result_staging + assert 'Platform: ECS' in result_staging + assert 'Application: staging-app' in result_staging + + # Verify they're different + assert 'prod-app' in result_prod and 'prod-app' not in result_staging + assert 'staging-app' in result_staging and 'staging-app' not in result_prod @pytest.mark.asyncio -async def test_query_service_metrics_success(mock_aws_clients): - """Test successful query of service metrics.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] +async def test_get_service_detail_passes_correct_key_attributes(mock_aws_clients): + """Verify KeyAttributes dict is constructed correctly from parameters.""" + mock_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'payment-service', + 'Type': 'RemoteService', + 'Environment': 'production', + }, + } + } + + mock_aws_clients['appsignals_client'].get_service.return_value = mock_response + + await get_service_detail( + service_name='payment-service', + environment='production', + service_type='RemoteService', + ) + + # Verify get_service was called with correct KeyAttributes + call_args = mock_aws_clients['appsignals_client'].get_service.call_args + assert call_args is not None + + key_attributes = call_args[1]['KeyAttributes'] + assert key_attributes == { + 'Type': 'RemoteService', + 'Name': 'payment-service', + 'Environment': 'production', + } + + # Verify all three fields are present and correct + assert key_attributes['Type'] == 'RemoteService' + assert key_attributes['Name'] == 'payment-service' + assert key_attributes['Environment'] == 'production' + + +@pytest.mark.asyncio +async def test_get_service_detail_remote_service_type(mock_aws_clients): + """Test get_service_detail with Type='RemoteService'.""" + mock_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'external-api', + 'Type': 'RemoteService', + 'Environment': 'production', + }, + 'AttributeMaps': [{'RemoteType': 'HTTP', 'RemoteTarget': 'api.example.com'}], + 'MetricReferences': [ + { + 'Namespace': 'AWS/ApplicationSignals', + 'MetricName': 'Latency', + 'Dimensions': [], + } + ], + } + } + + mock_aws_clients['appsignals_client'].get_service.return_value = mock_response + + result = await get_service_detail( + service_name='external-api', + environment='production', + service_type='RemoteService', + ) + + assert 'Service Details: external-api' in result + assert 'Type: RemoteService' in result + assert 'Environment: production' in result + assert 'RemoteType: HTTP' in result + assert 'RemoteTarget: api.example.com' in result + + +@pytest.mark.asyncio +async def test_get_service_detail_aws_service_type(mock_aws_clients): + """Test get_service_detail with Type='AWS::Service'.""" + mock_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'dynamodb', + 'Type': 'AWS::Service', + 'Environment': 'production', + }, + 'AttributeMaps': [{'ResourceType': 'AWS::DynamoDB::Table', 'TableName': 'users'}], + 'MetricReferences': [ + { + 'Namespace': 'AWS/ApplicationSignals', + 'MetricName': 'Fault', + 'Dimensions': [], + } + ], + } + } + + mock_aws_clients['appsignals_client'].get_service.return_value = mock_response + + result = await get_service_detail( + service_name='dynamodb', + environment='production', + service_type='AWS::Service', + ) + + assert 'Service Details: dynamodb' in result + assert 'Type: AWS::Service' in result + assert 'Environment: production' in result + assert 'ResourceType: AWS::DynamoDB::Table' in result + assert 'TableName: users' in result + + +@pytest.mark.asyncio +async def test_key_attributes_structure_validation(mock_aws_clients): + """Verify KeyAttributes dict has exactly Type, Name, Environment with correct values.""" + mock_response = { + 'Service': { + 'KeyAttributes': { + 'Name': 'test-service', + 'Type': 'Service', + 'Environment': 'dev', + }, + } } + mock_aws_clients['appsignals_client'].get_service.return_value = mock_response + + await get_service_detail( + service_name='test-service', environment='dev', service_type='Service' + ) + + # Get the call arguments + call_args = mock_aws_clients['appsignals_client'].get_service.call_args + key_attributes = call_args[1]['KeyAttributes'] + + # Verify structure: exactly 3 keys + assert len(key_attributes) == 3, f'Expected 3 keys, got {len(key_attributes)}' + + # Verify required keys exist + assert 'Type' in key_attributes, 'Missing required key: Type' + assert 'Name' in key_attributes, 'Missing required key: Name' + assert 'Environment' in key_attributes, 'Missing required key: Environment' + + # Verify no extra keys + expected_keys = {'Type', 'Name', 'Environment'} + actual_keys = set(key_attributes.keys()) + assert actual_keys == expected_keys, f'Extra keys found: {actual_keys - expected_keys}' + + # Verify types are strings + assert isinstance(key_attributes['Type'], str), 'Type must be a string' + assert isinstance(key_attributes['Name'], str), 'Name must be a string' + assert isinstance(key_attributes['Environment'], str), 'Environment must be a string' + + # Verify values match parameters + assert key_attributes['Type'] == 'Service' + assert key_attributes['Name'] == 'test-service' + assert key_attributes['Environment'] == 'dev' + + +@pytest.mark.asyncio +async def test_query_service_metrics_success(mock_aws_clients): + """Test successful query of service metrics.""" mock_get_response = { 'Service': { 'MetricReferences': [ @@ -225,12 +431,12 @@ async def test_query_service_metrics_success(mock_aws_clients): ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response mock_aws_clients['cloudwatch_client'].get_metric_statistics.return_value = mock_metric_response result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -245,12 +451,6 @@ async def test_query_service_metrics_success(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_list_available(mock_aws_clients): """Test listing available metrics when no specific metric is requested.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = { 'Service': { 'MetricReferences': [ @@ -268,11 +468,11 @@ async def test_query_service_metrics_list_available(mock_aws_clients): } } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='', # Empty to list available metrics statistic='Average', extended_statistic='p99', @@ -678,13 +878,6 @@ async def test_list_monitored_services_general_exception(mock_aws_clients): @pytest.mark.asyncio async def test_get_service_detail_client_error(mock_aws_clients): """Test ClientError handling in get_service_detail.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.side_effect = ClientError( error_response={ 'Error': { @@ -695,7 +888,7 @@ async def test_get_service_detail_client_error(mock_aws_clients): operation_name='GetService', ) - result = await get_service_detail('test-service') + result = await get_service_detail(service_name='test-service', environment='production') assert 'AWS Error: Service not found in Application Signals' in result @@ -703,18 +896,11 @@ async def test_get_service_detail_client_error(mock_aws_clients): @pytest.mark.asyncio async def test_get_service_detail_general_exception(mock_aws_clients): """Test general exception handling in get_service_detail.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.side_effect = Exception( 'Unexpected error in get_service' ) - result = await get_service_detail('test-service') + result = await get_service_detail(service_name='test-service', environment='production') assert 'Error: Unexpected error in get_service' in result @@ -722,12 +908,6 @@ async def test_get_service_detail_general_exception(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_no_datapoints(mock_aws_clients): """Test query service metrics when no datapoints are returned.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = { 'Service': { 'MetricReferences': [ @@ -742,12 +922,12 @@ async def test_query_service_metrics_no_datapoints(mock_aws_clients): mock_metric_response = {'Datapoints': []} - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response mock_aws_clients['cloudwatch_client'].get_metric_statistics.return_value = mock_metric_response result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -917,37 +1097,39 @@ async def test_list_slis_with_error_in_sli_client(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_service_not_found(mock_aws_clients): """Test query service metrics when service is not found.""" - mock_list_response = {'ServiceSummaries': []} - - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response + mock_aws_clients['appsignals_client'].get_service.side_effect = ClientError( + error_response={ + 'Error': { + 'Code': 'ResourceNotFoundException', + 'Message': 'Service not found', + } + }, + operation_name='GetService', + ) result = await query_service_metrics( service_name='nonexistent-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', hours=1, ) - assert "Service 'nonexistent-service' not found" in result + assert 'AWS Error: Service not found' in result + assert "Service not found with Name='nonexistent-service', Environment='production'" in result @pytest.mark.asyncio async def test_query_service_metrics_no_metrics(mock_aws_clients): """Test query service metrics when service has no metrics.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = {'Service': {'MetricReferences': []}} - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -960,12 +1142,6 @@ async def test_query_service_metrics_no_metrics(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_metric_not_found(mock_aws_clients): """Test query service metrics when specific metric is not found.""" - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = { 'Service': { 'MetricReferences': [ @@ -978,11 +1154,11 @@ async def test_query_service_metrics_metric_not_found(mock_aws_clients): } } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', # Looking for Latency but only Error exists statistic='Average', extended_statistic='p99', @@ -996,18 +1172,19 @@ async def test_query_service_metrics_metric_not_found(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_client_error(mock_aws_clients): """Test query service metrics with client error.""" - mock_aws_clients['appsignals_client'].list_services.side_effect = ClientError( + mock_aws_clients['appsignals_client'].get_service.side_effect = ClientError( error_response={ 'Error': { 'Code': 'AccessDeniedException', 'Message': 'User is not authorized', } }, - operation_name='ListServices', + operation_name='GetService', ) result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -1139,12 +1316,6 @@ async def test_query_service_metrics_different_periods(mock_aws_clients): ] for hours, expected_period in test_cases: - mock_list_response = { - 'ServiceSummaries': [ - {'KeyAttributes': {'Name': 'test-service', 'Type': 'AWS::ECS::Service'}} - ] - } - mock_get_response = { 'Service': { 'MetricReferences': [ @@ -1161,7 +1332,6 @@ async def test_query_service_metrics_different_periods(mock_aws_clients): 'Datapoints': [{'Timestamp': datetime.now(timezone.utc), 'Average': 100.0}] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_list_response mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response mock_aws_clients[ 'cloudwatch_client' @@ -1169,6 +1339,7 @@ async def test_query_service_metrics_different_periods(mock_aws_clients): await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -1183,10 +1354,11 @@ async def test_query_service_metrics_different_periods(mock_aws_clients): @pytest.mark.asyncio async def test_query_service_metrics_general_exception(mock_aws_clients): """Test query service metrics with unexpected exception.""" - mock_aws_clients['appsignals_client'].list_services.side_effect = Exception('Unexpected error') + mock_aws_clients['appsignals_client'].get_service.side_effect = Exception('Unexpected error') result = await query_service_metrics( service_name='test-service', + environment='production', metric_name='Latency', statistic='Average', extended_statistic='p99', @@ -1196,6 +1368,136 @@ async def test_query_service_metrics_general_exception(mock_aws_clients): assert 'Error: Unexpected error' in result +@pytest.mark.asyncio +async def test_query_service_metrics_distinguishes_environments(mock_aws_clients): + """Test that we can query metrics for same service name in different environments.""" + # Mock response for production environment + mock_prod_get_response = { + 'Service': { + 'MetricReferences': [ + { + 'Namespace': 'AWS/ApplicationSignals', + 'MetricName': 'Latency', + 'Dimensions': [], + } + ] + } + } + + # Mock response for staging environment + mock_staging_get_response = { + 'Service': { + 'MetricReferences': [ + { + 'Namespace': 'AWS/ApplicationSignals', + 'MetricName': 'Latency', + 'Dimensions': [], + } + ] + } + } + + # Mock metrics with different values for each environment + mock_prod_metric_response = { + 'Datapoints': [{'Timestamp': datetime.now(timezone.utc), 'Average': 150.0, 'p99': 500.0}] + } + + mock_staging_metric_response = { + 'Datapoints': [{'Timestamp': datetime.now(timezone.utc), 'Average': 50.0, 'p99': 100.0}] + } + + # Test production environment + mock_aws_clients['appsignals_client'].get_service.return_value = mock_prod_get_response + mock_aws_clients[ + 'cloudwatch_client' + ].get_metric_statistics.return_value = mock_prod_metric_response + + result_prod = await query_service_metrics( + service_name='api-service', + environment='production', + metric_name='Latency', + statistic='Average', + extended_statistic='p99', + hours=1, + ) + + assert 'Metrics for api-service - Latency' in result_prod + assert '150.00' in result_prod + assert '500.00' in result_prod + + # Test staging environment + mock_aws_clients['appsignals_client'].get_service.return_value = mock_staging_get_response + mock_aws_clients[ + 'cloudwatch_client' + ].get_metric_statistics.return_value = mock_staging_metric_response + + result_staging = await query_service_metrics( + service_name='api-service', + environment='staging', + metric_name='Latency', + statistic='Average', + extended_statistic='p99', + hours=1, + ) + + assert 'Metrics for api-service - Latency' in result_staging + assert '50.00' in result_staging + assert '100.00' in result_staging + + # Verify they have different values (use more specific checks to avoid substring matches) + assert 'Latest: 150.00' in result_prod and 'Latest: 150.00' not in result_staging + assert 'Latest: 50.00' in result_staging and 'Latest: 50.00' not in result_prod + + +@pytest.mark.asyncio +async def test_query_service_metrics_passes_correct_key_attributes(mock_aws_clients): + """Verify KeyAttributes dict is constructed correctly from parameters.""" + mock_get_response = { + 'Service': { + 'MetricReferences': [ + { + 'Namespace': 'AWS/ApplicationSignals', + 'MetricName': 'Error', + 'Dimensions': [], + } + ] + } + } + + mock_metric_response = { + 'Datapoints': [{'Timestamp': datetime.now(timezone.utc), 'Average': 5.0}] + } + + mock_aws_clients['appsignals_client'].get_service.return_value = mock_get_response + mock_aws_clients['cloudwatch_client'].get_metric_statistics.return_value = mock_metric_response + + await query_service_metrics( + service_name='payment-service', + environment='staging', + service_type='AWS::Service', + metric_name='Error', + statistic='Average', + extended_statistic='p99', + hours=1, + ) + + # Verify get_service was called with correct KeyAttributes + call_args = mock_aws_clients['appsignals_client'].get_service.call_args + assert call_args is not None + + key_attributes = call_args[1]['KeyAttributes'] + assert key_attributes == { + 'Type': 'AWS::Service', + 'Name': 'payment-service', + 'Environment': 'staging', + } + + # Verify all three fields are present and correct + assert key_attributes['Type'] == 'AWS::Service' + assert key_attributes['Name'] == 'payment-service' + assert key_attributes['Environment'] == 'staging' + + @pytest.mark.asyncio async def test_search_transaction_spans_general_exception(mock_aws_clients): """Test search transaction spans with general exception.""" diff --git a/src/cloudwatch-appsignals-mcp-server/tests/test_service_tools_operations.py b/src/cloudwatch-appsignals-mcp-server/tests/test_service_tools_operations.py index 352e3d430a..cda5f4233e 100644 --- a/src/cloudwatch-appsignals-mcp-server/tests/test_service_tools_operations.py +++ b/src/cloudwatch-appsignals-mcp-server/tests/test_service_tools_operations.py @@ -39,19 +39,6 @@ def mock_aws_clients(): @pytest.mark.asyncio async def test_list_service_operations_success_with_get_operations(mock_aws_clients): """Test successful list_service_operations with GET operations.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'payment-service', - 'Type': 'AWS::ECS::Service', - 'Environment': 'production', - } - } - ] - } - # Mock operations response with GET operations mock_operations_response = { 'ServiceOperations': [ @@ -78,12 +65,13 @@ async def test_list_service_operations_success_with_get_operations(mock_aws_clie ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='payment-service', hours=12) + result = await list_service_operations( + service_name='payment-service', environment='production', hours=12 + ) assert 'Operations for Service: payment-service' in result assert 'Time Range: Last 12 hour(s)' in result @@ -106,18 +94,6 @@ async def test_list_service_operations_success_with_get_operations(mock_aws_clie @pytest.mark.asyncio async def test_list_service_operations_success_with_other_operations(mock_aws_clients): """Test successful list_service_operations with other operation types.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'api-service', - 'Type': 'AWS::Lambda::Function', - } - } - ] - } - # Mock operations response with various operation types mock_operations_response = { 'ServiceOperations': [ @@ -141,12 +117,13 @@ async def test_list_service_operations_success_with_other_operations(mock_aws_cl ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='api-service', hours=24) + result = await list_service_operations( + service_name='api-service', environment='staging', hours=24 + ) assert 'Operations for Service: api-service' in result assert 'Total Operations: 3' in result @@ -167,27 +144,16 @@ async def test_list_service_operations_success_with_other_operations(mock_aws_cl @pytest.mark.asyncio async def test_list_service_operations_no_operations_found(mock_aws_clients): """Test list_service_operations when no operations are found.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'inactive-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock empty operations response mock_operations_response = {'ServiceOperations': []} - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='inactive-service', hours=6) + result = await list_service_operations( + service_name='inactive-service', environment='production', hours=6 + ) assert "No operations found for service 'inactive-service' in the last 6 hours" in result assert ( @@ -205,32 +171,29 @@ async def test_list_service_operations_no_operations_found(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_service_not_found(mock_aws_clients): """Test list_service_operations when service is not found.""" - # Mock empty services response - mock_services_response = {'ServiceSummaries': []} - - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response + # Mock ClientError for service not found + mock_aws_clients['appsignals_client'].list_service_operations.side_effect = ClientError( + error_response={ + 'Error': { + 'Code': 'ResourceNotFoundException', + 'Message': 'Service not found', + } + }, + operation_name='ListServiceOperations', + ) - result = await list_service_operations(service_name='nonexistent-service', hours=24) + result = await list_service_operations( + service_name='nonexistent-service', environment='production', hours=24 + ) - assert "Service 'nonexistent-service' not found in Application Signals" in result + assert 'AWS Error: Service not found' in result + assert "Service not found with Name='nonexistent-service', Environment='production'" in result assert 'Use list_monitored_services() to see available services' in result @pytest.mark.asyncio async def test_list_service_operations_hours_limit_enforcement(mock_aws_clients): """Test that hours parameter is limited to 24 hours maximum.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response mock_operations_response = { 'ServiceOperations': [ @@ -241,13 +204,14 @@ async def test_list_service_operations_hours_limit_enforcement(mock_aws_clients) ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response # Request 48 hours but should be limited to 24 - result = await list_service_operations(service_name='test-service', hours=48) + result = await list_service_operations( + service_name='test-service', environment='production', hours=48 + ) assert 'Time Range: Last 24 hour(s)' in result # Should be limited to 24 @@ -262,18 +226,6 @@ async def test_list_service_operations_hours_limit_enforcement(mock_aws_clients) @pytest.mark.asyncio async def test_list_service_operations_mixed_operation_types(mock_aws_clients): """Test list_service_operations with mixed GET, POST, and other operations.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'mixed-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response with mixed types mock_operations_response = { 'ServiceOperations': [ @@ -304,12 +256,13 @@ async def test_list_service_operations_mixed_operation_types(mock_aws_clients): ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='mixed-service', hours=24) + result = await list_service_operations( + service_name='mixed-service', environment='production', hours=24 + ) assert 'Total Operations: 6' in result assert '🔍 GET Operations (2):' in result @@ -329,18 +282,6 @@ async def test_list_service_operations_mixed_operation_types(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_operations_without_metrics(mock_aws_clients): """Test list_service_operations with operations that have no metric references.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response with operations without metrics mock_operations_response = { 'ServiceOperations': [ @@ -355,12 +296,13 @@ async def test_list_service_operations_operations_without_metrics(mock_aws_clien ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'Total Operations: 2' in result assert 'GET /api/health' in result @@ -380,17 +322,19 @@ async def test_list_service_operations_operations_without_metrics(mock_aws_clien @pytest.mark.asyncio async def test_list_service_operations_client_error(mock_aws_clients): """Test list_service_operations with AWS ClientError.""" - mock_aws_clients['appsignals_client'].list_services.side_effect = ClientError( + mock_aws_clients['appsignals_client'].list_service_operations.side_effect = ClientError( error_response={ 'Error': { 'Code': 'AccessDeniedException', 'Message': 'User is not authorized to perform this action', } }, - operation_name='ListServices', + operation_name='ListServiceOperations', ) - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'AWS Error: User is not authorized to perform this action' in result @@ -398,11 +342,13 @@ async def test_list_service_operations_client_error(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_general_exception(mock_aws_clients): """Test list_service_operations with general exception.""" - mock_aws_clients['appsignals_client'].list_services.side_effect = Exception( + mock_aws_clients['appsignals_client'].list_service_operations.side_effect = Exception( 'Unexpected error occurred' ) - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'Error: Unexpected error occurred' in result @@ -410,20 +356,6 @@ async def test_list_service_operations_general_exception(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_operations_api_client_error(mock_aws_clients): """Test list_service_operations when list_service_operations API fails.""" - # Mock successful service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response - # Mock failure in list_service_operations API mock_aws_clients['appsignals_client'].list_service_operations.side_effect = ClientError( error_response={ @@ -435,7 +367,9 @@ async def test_list_service_operations_operations_api_client_error(mock_aws_clie operation_name='ListServiceOperations', ) - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'AWS Error: Rate exceeded' in result @@ -443,18 +377,6 @@ async def test_list_service_operations_operations_api_client_error(mock_aws_clie @pytest.mark.asyncio async def test_list_service_operations_duplicate_metric_types(mock_aws_clients): """Test list_service_operations with duplicate metric types (should be deduplicated).""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response with duplicate metric types mock_operations_response = { 'ServiceOperations': [ @@ -471,12 +393,13 @@ async def test_list_service_operations_duplicate_metric_types(mock_aws_clients): ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'GET /api/test' in result # Should deduplicate metric types using set() @@ -495,18 +418,6 @@ async def test_list_service_operations_duplicate_metric_types(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_unknown_operation_name(mock_aws_clients): """Test list_service_operations with operations that have missing or unknown names.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response with missing/unknown operation names mock_operations_response = { 'ServiceOperations': [ @@ -525,12 +436,13 @@ async def test_list_service_operations_unknown_operation_name(mock_aws_clients): ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'Total Operations: 3' in result assert 'GET /api/test' in result @@ -543,18 +455,6 @@ async def test_list_service_operations_unknown_operation_name(mock_aws_clients): @pytest.mark.asyncio async def test_list_service_operations_unknown_metric_types(mock_aws_clients): """Test list_service_operations with operations that have unknown metric types.""" - # Mock service discovery - mock_services_response = { - 'ServiceSummaries': [ - { - 'KeyAttributes': { - 'Name': 'test-service', - 'Type': 'AWS::ECS::Service', - } - } - ] - } - # Mock operations response with unknown metric types mock_operations_response = { 'ServiceOperations': [ @@ -569,12 +469,13 @@ async def test_list_service_operations_unknown_metric_types(mock_aws_clients): ] } - mock_aws_clients['appsignals_client'].list_services.return_value = mock_services_response mock_aws_clients[ 'appsignals_client' ].list_service_operations.return_value = mock_operations_response - result = await list_service_operations(service_name='test-service', hours=24) + result = await list_service_operations( + service_name='test-service', environment='production', hours=24 + ) assert 'GET /api/test' in result assert 'Available Metrics:' in result @@ -582,3 +483,102 @@ async def test_list_service_operations_unknown_metric_types(mock_aws_clients): metrics_line = next(line for line in result.split('\n') if 'Available Metrics:' in line) assert 'Latency' in metrics_line assert 'Unknown' in metrics_line # Should show "Unknown" for missing/empty metric types + + +@pytest.mark.asyncio +async def test_list_service_operations_distinguishes_environments(mock_aws_clients): + """Test that we can list operations for same service name in different environments.""" + # Mock operations for production environment + mock_prod_operations = { + 'ServiceOperations': [ + { + 'Name': 'GET /api/orders', + 'MetricReferences': [{'MetricType': 'Latency'}], + }, + { + 'Name': 'POST /api/checkout', + 'MetricReferences': [{'MetricType': 'Latency'}], + }, + ] + } + + # Mock operations for staging environment + mock_staging_operations = { + 'ServiceOperations': [ + { + 'Name': 'GET /api/test', + 'MetricReferences': [{'MetricType': 'Latency'}], + }, + ] + } + + # Test production environment + mock_aws_clients[ + 'appsignals_client' + ].list_service_operations.return_value = mock_prod_operations + + result_prod = await list_service_operations( + service_name='payment-service', environment='production', hours=24 + ) + + assert 'Operations for Service: payment-service' in result_prod + assert 'Total Operations: 2' in result_prod + assert 'GET /api/orders' in result_prod + assert 'POST /api/checkout' in result_prod + + # Test staging environment + mock_aws_clients[ + 'appsignals_client' + ].list_service_operations.return_value = mock_staging_operations + + result_staging = await list_service_operations( + service_name='payment-service', environment='staging', hours=24 + ) + + assert 'Operations for Service: payment-service' in result_staging + assert 'Total Operations: 1' in result_staging + assert 'GET /api/test' in result_staging + + # Verify they have different operations + assert 'GET /api/orders' in result_prod and 'GET /api/orders' not in result_staging + assert 'GET /api/test' in result_staging and 'GET /api/test' not in result_prod + + +@pytest.mark.asyncio +async def test_list_service_operations_passes_correct_key_attributes(mock_aws_clients): + """Verify KeyAttributes dict is constructed correctly from parameters.""" + mock_operations_response = { + 'ServiceOperations': [ + { + 'Name': 'GET /health', + 'MetricReferences': [{'MetricType': 'Latency'}], + } + ] + } + + mock_aws_clients[ + 'appsignals_client' + ].list_service_operations.return_value = mock_operations_response + + await list_service_operations( + service_name='api-gateway', + environment='production', + service_type='RemoteService', + hours=12, + ) + + # Verify list_service_operations was called with correct KeyAttributes + call_args = mock_aws_clients['appsignals_client'].list_service_operations.call_args + assert call_args is not None + + key_attributes = call_args[1]['KeyAttributes'] + assert key_attributes == { + 'Type': 'RemoteService', + 'Name': 'api-gateway', + 'Environment': 'production', + } + + # Verify all three fields are present and correct + assert key_attributes['Type'] == 'RemoteService' + assert key_attributes['Name'] == 'api-gateway' + assert key_attributes['Environment'] == 'production'