Skip to content

Feat/update config#430

Merged
jeastham1993 merged 11 commits intomainfrom
feat/update-config
Sep 1, 2025
Merged

Feat/update config#430
jeastham1993 merged 11 commits intomainfrom
feat/update-config

Conversation

@jeastham1993
Copy link
Copy Markdown
Collaborator

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Types of Changes

  • Bug fix
  • New feature
  • Breaking change
  • Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

  • This PR's description is comprehensive
  • This PR contains breaking changes that are documented in the description
  • This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
  • This PR impacts documentation, and it has been updated (or a ticket has been logged)
  • This PR's changes are covered by the automated tests
  • This PR collects user input/sensitive content into Datadog

Copilot AI review requested due to automatic review settings September 1, 2025 09:33
@claude
Copy link
Copy Markdown

claude bot commented Sep 1, 2025

Claude finished @jeastham1993's task —— View job


Code Review Complete ✅

I've completed a thorough review of this configuration update PR that bumps Datadog layer versions and removes some Datadog configuration.

Summary

This PR primarily updates Datadog extension and layer versions across all serverless services from version 83→85 (extension) and various language layer versions. It also removes Datadog configuration from the pricing service stack.

✅ Positive Aspects

  1. Consistent Version Updates: All services consistently update extension layer to version 85
  2. Multi-Platform Coverage: Updates span Python, Node.js, Java, and Go services across CDK, SAM, and Terraform
  3. Infrastructure-as-Code Best Practices: Uses proper parameterization and variable references

⚠️ Issues Found

Critical: Inconsistent Layer Versions

  • Issue: src/activity-service/infra/modules/python-lambda-function/main.tf:111 still references Python layer version 111 while others use 113
  • Impact: This service will use an outdated Datadog Python layer, potentially missing bug fixes or features
  • Fix: Update line 111 to datadog_python_layer_version = 113

Critical: Inconsistent Node.js Layer Versions

  • Issue: CDK stacks use Node.js layer version 125, but SAM templates and Terraform use 127
  • Files Affected:
    • src/loyalty-point-service/lib/loyalty-api/loyaltyApiStack.ts:34 (uses 125)
    • src/order-mcp/lib/order-mcp/orderMcpStack.ts (uses 125)
    • While corresponding SAM/Terraform files use 127
  • Impact: Inconsistent Datadog instrumentation across deployment methods for the same service
  • Fix: Standardize on version 127 across all deployment methods

Major: Incomplete Datadog Removal in Pricing Service

  • Issue: src/pricing-service/lib/pricing-api/pricingApiStack.ts removes Datadog configuration but sets datadogConfiguration: undefined
  • Impact:
    • Pricing service loses observability
    • No error handling/fallback monitoring
    • Dead code remains (unused imports, incomplete cleanup)
  • Recommendation: Either:
    • Restore Datadog configuration if observability is needed
    • Complete the removal by cleaning up related code and ensuring alternative monitoring

Moderate: Missing Error Handling

  • Issue: No validation that layer versions are compatible with runtime versions
  • Impact: Deployment could fail if layer versions are incompatible with Lambda runtimes
  • Recommendation: Add validation or compatibility checks

Security: Terraform Logging Policy Issue

  • Issue: src/inventory-service/infra/modules/lambda-function/main.tf:33-45 has duplicate logs:CreateLogStream action in deny policy
  • Impact: Minor - doesn't affect functionality but indicates copy-paste errors
  • Fix: Remove duplicate action on line 40

📋 Recommendations

  1. Fix Version Inconsistencies: Ensure all services use the same layer versions across deployment methods
  2. Complete Pricing Service Changes: Either restore Datadog config or properly remove all related code
  3. Add Validation: Consider adding checks for layer/runtime compatibility
  4. Update Documentation: Document the version changes and any breaking changes
  5. Test Coverage: Ensure all services deploy successfully with new layer versions

🔧 Files Requiring Changes

High Priority:

  • src/activity-service/infra/modules/python-lambda-function/main.tf (line 111)
  • src/loyalty-point-service/lib/loyalty-api/loyaltyApiStack.ts (line 34)
  • src/order-mcp/lib/order-mcp/orderMcpStack.ts (Node.js layer version)
  • src/pricing-service/lib/pricing-api/pricingApiStack.ts (incomplete Datadog removal)

Medium Priority:

  • src/inventory-service/infra/modules/lambda-function/main.tf (duplicate log action)

💡 Overall Assessment

This is a maintenance PR that properly updates Datadog layers across the microservices architecture. The changes are necessary for keeping instrumentation current, but several inconsistencies need to be addressed before merging to ensure all services have proper observability and consistent configuration.

Recommendation: Request changes to fix the version inconsistencies and complete the pricing service modifications.

Copy link
Copy Markdown
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 updates Datadog Lambda layer configurations across multiple services in the serverless sample application. It upgrades extension layer versions from various older versions to version 85, node layer versions from 125 to 127, and Python layer versions from 111 to 113. Additionally, it removes the deprecated DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED environment variable across all services.

  • Updates Datadog extension layer to version 85 across all services
  • Updates node and Python layer versions to latest supported versions
  • Removes deprecated Datadog trace configuration setting

Reviewed Changes

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

Show a summary per file
File Description
src/user-management-service/template.yaml Updates extension layer version and removes deprecated environment variable
src/user-management-service/lib/user-management-api/user-management-stack.ts Updates extension layer version in CDK configuration
src/user-management-service/infra/modules/lambda-function/main.tf Updates Terraform configuration for extension layer
src/user-management-service/README.md Updates documentation examples with new layer versions
src/shared-infra/README.md Updates documentation examples with new layer versions
src/product-management-service/template.yaml Updates ARM extension layer version and removes deprecated environment variable
src/product-management-service/infra/modules/lambda-function/main.tf Updates Terraform configuration for extension layer
src/product-management-service/cdk/sharedConstructs/instrumentedFunction.go Removes deprecated environment variable from Go CDK construct
src/product-management-service/cdk/cdk.go Updates extension layer version in Go CDK configuration
src/product-management-service/README.md Updates documentation examples with new layer versions
src/pricing-service/template.yaml Removes deprecated environment variable from template
src/pricing-service/lib/pricing-api/pricingApiStack.ts Removes Datadog configuration and event handlers setup
src/pricing-service/infra/services/pricing/eventHandlers.tf Removes deprecated environment variable from commented Terraform
src/pricing-service/infra/modules/lambda-function/main.tf Removes deprecated environment variable from Terraform
src/pricing-service/README.md Updates documentation examples with new layer versions
src/order-service/template.yaml Updates extension layer version and removes deprecated environment variable
src/order-service/infra/modules/lambda-function/main.tf Updates Terraform configuration for extension layer
src/order-service/cdk/Constructs/InstrumentedFunction.cs Updates ARM extension layer version in C# CDK construct
src/order-service/README.md Updates documentation examples with new layer versions
src/order-mcp/template.yaml Updates node and extension layer versions, removes deprecated environment variable
src/order-mcp/lib/order-mcp/orderMcpStack.ts Updates extension layer version in CDK configuration
src/order-mcp/infra/modules/lambda-function/main.tf Updates Terraform configuration for both layer versions
src/order-mcp/README.md Updates documentation examples with new layer versions
src/loyalty-point-service/template.yaml Updates node and extension layer versions, removes deprecated environment variable
src/loyalty-point-service/lib/loyalty-api/loyaltyApiStack.ts Updates extension layer version in CDK configuration
src/loyalty-point-service/infra/modules/lambda-function/main.tf Updates Terraform configuration for both layer versions
src/loyalty-point-service/README.md Updates documentation examples with new layer versions
src/inventory-service/template.yaml Updates extension layer version and removes deprecated environment variable
src/inventory-service/infra/modules/lambda-function/main.tf Updates Terraform configuration for extension layer
src/inventory-service/cdk/src/main/java/com/cdk/constructs/InstrumentedFunction.java Updates extension layer version in Java CDK construct
src/inventory-service/README.md Updates documentation examples with new layer versions
src/activity-service/template.yaml Updates Python and extension layer versions
src/activity-service/infra/modules/python-lambda-function/main.tf Updates extension layer version and removes deprecated environment variable
src/activity-service/infra/modules/lambda-function/main.tf Corrects runtime and layer configurations for Python service
src/activity-service/cdk/activity_service/service_stack.py Updates Python and extension layer versions in Python CDK

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.


filename = var.zip_file
function_name = "tf-node-${var.function_name}-${var.env}"
function_name = "tf-python-${var.function_name}-${var.env}"
Copy link

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

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

The function name prefix and runtime are being changed from Node.js to Python in what appears to be a generic lambda function module. This could break existing infrastructure or indicate the wrong module is being modified.

Copilot uses AI. Check for mistakes.
role = aws_iam_role.lambda_function_role.arn
handler = var.lambda_handler
runtime = "nodejs22.x"
runtime = "python3.13"
Copy link

Copilot AI Sep 1, 2025

Choose a reason for hiding this comment

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

The function name prefix and runtime are being changed from Node.js to Python in what appears to be a generic lambda function module. This could break existing infrastructure or indicate the wrong module is being modified.

Copilot uses AI. Check for mistakes.
@claude
Copy link
Copy Markdown

claude bot commented Sep 1, 2025

Claude finished @jeastham1993's task —— View job


Comprehensive Code Review Complete ✅

I've conducted a thorough review of this Datadog configuration update PR. Here's my detailed analysis across all requested areas:

Summary

This PR updates Datadog layer versions across all serverless services (extension layer 83→85, Node.js layer 125→127, Python layer 111→113) and removes the deprecated DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED environment variable.

Good Progress Made

  1. Deprecated Environment Variable Removed: ✅ Successfully removed DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED across all services
  2. Consistent Extension Layer Updates: ✅ All services properly updated to extension layer version 85
  3. Documentation Updated: ✅ README files updated with new layer versions
  4. Multi-Platform Coverage: ✅ Updates span CDK, SAM, and Terraform consistently

🚨 Critical Issues Remain

1. Version Inconsistency in Python Layer (High Priority)

  • File: src/activity-service/infra/modules/python-lambda-function/main.tf:132
  • Issue: Still uses Python layer version 111, while SAM template and CDK use 113
  • Impact: This Terraform deployment will use outdated Datadog instrumentation
  • Risk: Missing bug fixes, features, or security updates in the layer

2. Node.js Layer Inconsistency (High Priority)

  • Files:
    • src/loyalty-point-service/lib/loyalty-api/loyaltyApiStack.ts:33FIXED (now 127)
    • src/order-mcp/lib/order-mcp/orderMcpStack.ts:32FIXED (now 127)
  • Status: Both CDK stacks now properly use version 127 - this issue has been resolved!

3. Pricing Service Observability Loss (Critical)

  • File: src/pricing-service/lib/pricing-api/pricingApiStack.ts:36
  • Issue: datadogConfiguration: undefined completely disables monitoring
  • Impact:
    • Total loss of observability for pricing service
    • No distributed tracing, metrics, or error tracking
    • Debugging production issues becomes extremely difficult
  • Recommendation: Either restore Datadog config or implement alternative monitoring

📋 Code Quality & Best Practices

Strengths

  • Consistent parameterization using environment variables
  • Proper use of Infrastructure-as-Code patterns
  • Good separation of concerns between deployment methods
  • Comprehensive update across all services

Areas for Improvement

  • Missing Validation: No checks for layer version compatibility with runtime versions
  • Incomplete Cleanup: Pricing service has unused imports and partial configuration removal
  • Documentation Gap: No explanation of why pricing service monitoring was removed

🔒 Security Assessment

  • Secrets Management: ✅ Proper use of AWS Secrets Manager for API keys
  • IAM Policies: ✅ Appropriate least-privilege permissions
  • Layer Updates: ✅ Generally good for security (newer layers include fixes)
  • No Security Concerns: No sensitive data exposure or new attack vectors introduced

Performance Considerations

  • Layer Size: Newer Datadog layers are typically more efficient
  • Cold Start: Extension layer 85 may have cold start improvements
  • Memory Usage: Layer updates shouldn't significantly impact function memory
  • Pricing Service: Removing Datadog will reduce memory overhead but at the cost of observability

🧪 Test Coverage Assessment

Good Coverage Found:

  • Python: Unit & integration tests in src/activity-service/tests/
  • Java: Comprehensive test suites in src/inventory-service/
  • C#: Unit & integration tests in src/order-service/
  • Go: Good unit test coverage in src/product-management-service/
  • TypeScript: End-to-end tests in multiple services

Missing: Tests specifically validating Datadog configuration updates, but this is acceptable for infrastructure changes.

🔧 Required Changes

Must Fix Before Merge:

  1. Python Layer Version: Update src/activity-service/infra/modules/python-lambda-function/main.tf:132 to version 113
  2. Pricing Service Decision: Either restore Datadog configuration or document why monitoring was intentionally removed

Recommended:
3. Add compatibility validation for layer versions
4. Clean up unused imports in pricing service if monitoring removal is intentional

💡 Final Recommendation

Request Changes - While this PR makes good progress on updating Datadog configurations, the critical Python layer inconsistency and pricing service observability loss need to be addressed before merging.

The Node.js layer inconsistencies mentioned in previous reviews have been successfully resolved! ✅


@claude
Copy link
Copy Markdown

claude bot commented Sep 1, 2025

Claude finished @jeastham1993's task —— View job


Code Review Complete ✅

I've completed a comprehensive review of this configuration update PR focusing on code quality, security, performance, and potential issues.

  • Code quality and best practices
  • Potential bugs or issues
  • Performance considerations
  • Security concerns
  • Test coverage

Summary

This PR updates Datadog layer versions across the serverless sample application, bumping extension layers to v85, Node.js layers to v127, and Python layers to v113. It also removes the deprecated DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED environment variable.

✅ Positive Aspects

  1. Consistent Configuration Updates: All services properly updated to latest Datadog layer versions
  2. Multi-Platform Coverage: Updates span Python, Node.js, Java, and Go services across CDK, SAM, and Terraform
  3. Proper Cleanup: Deprecated environment variable removed consistently
  4. Infrastructure-as-Code Best Practices: Uses proper parameterization and variable references

⚠️ Issues Found

Critical: Incomplete Datadog Removal in Pricing Service

  • Location: src/pricing-service/lib/pricing-api/pricingApiStack.ts:36
  • Issue: Sets datadogConfiguration: undefined which completely disables monitoring for this service
  • Impact:
    • Loss of observability for the pricing service
    • No distributed tracing or metrics collection
    • Potential production monitoring blind spot
  • Comments: Lines 29 and 47 suggest this is temporary with "Paste configuration here" placeholders
  • Recommendation: Either restore proper Datadog configuration or document why monitoring is intentionally disabled

Security: Duplicate IAM Policy Action

  • Location: src/inventory-service/infra/modules/lambda-function/main.tf:39-40
  • Issue: Duplicate logs:CreateLogStream action in deny policy
Action : [
  "logs:CreateLogStream",
  "logs:CreateLogStream",  // <- Duplicate
  "logs:PutLogEvents"
]
  • Impact: Minor - doesn't affect functionality but indicates copy-paste errors
  • Fix: Remove duplicate action on line 40

Code Quality: Python Layer Version Inconsistency (FIXED)

  • Previous Issue: Earlier reviews noted Python layer version 111 vs 113 inconsistency
  • Status: ✅ RESOLVED - All configurations now consistently use Python layer version 113
  • Verified Locations:
    • SAM template: src/activity-service/template.yaml:15
    • Terraform: src/activity-service/infra/modules/python-lambda-function/main.tf:132

Code Quality: Node.js Layer Version Consistency (FIXED)

  • Previous Issue: Earlier reviews noted Node.js layer version inconsistency (125 vs 127)
  • Status: ✅ RESOLVED - All configurations now consistently use Node.js layer version 127
  • Verified Locations:
    • CDK stacks: All use nodeLayerVersion: 127
    • SAM templates: All use nodeLayerVersion: 127
    • Terraform modules: All use appropriate version ✓

🔒 Security Assessment

  1. IAM Policies: Generally well-configured with least privilege principles
  2. Secret Management: Proper use of AWS Secrets Manager for API keys
  3. Resource Access: Appropriate scoping of permissions
  4. Logging Policy: Minor duplicate action issue (non-critical)

⚡ Performance Considerations

  1. Layer Versions: Updates to latest versions should include performance improvements
  2. Extension Overhead: Datadog extension v85 maintains low cold start impact
  3. Resource Configuration: No changes to memory/timeout settings (appropriate)

🧪 Test Coverage

  • Missing: No automated tests to validate layer compatibility with runtimes
  • Recommendation: Consider adding integration tests to ensure new layer versions work with existing Lambda functions
  • Deploy Testing: Ensure all services deploy successfully with new layer versions before production release

📋 Final Recommendations

High Priority

  1. Fix Pricing Service: Either restore Datadog configuration or properly document the intentional removal
  2. Clean up duplicate IAM action: Remove duplicate logs:CreateLogStream in inventory service Terraform

Medium Priority

  1. Add validation: Consider runtime/layer compatibility checks
  2. Documentation: Update any architectural docs referencing old layer versions
  3. Testing: Validate all services deploy and function correctly with new layers

Low Priority

  1. Consistency: Ensure all comments and documentation reflect the new versions

🎯 Overall Assessment

This is a solid maintenance PR that successfully updates Datadog instrumentation across a complex microservices architecture. The version inconsistencies previously identified have been resolved, and the changes follow infrastructure-as-code best practices.

Primary concerns:

  • Incomplete Datadog removal in pricing service needs attention
  • Minor IAM policy cleanup needed

Recommendation: Approve with minor fixes - The pricing service configuration should be completed and the duplicate IAM action removed before merging.


@claude
Copy link
Copy Markdown

claude bot commented Sep 1, 2025

Claude finished @jeastham1993's task —— View job


Comprehensive Code Review Complete ✅

I've thoroughly analyzed this Datadog configuration update PR across all requested areas:

  • Code quality and best practices
  • Potential bugs or issues
  • Performance considerations
  • Security concerns
  • Test coverage

Critical Issues Found 🚨

1. Version Inconsistencies Across CDK Files

Both CDK TypeScript stacks are using consistent Node.js layer version 127, which is correct. However, previous comments incorrectly identified these as inconsistent:

  • src/loyalty-point-service/lib/loyalty-api/loyaltyApiStack.ts:33Correct: nodeLayerVersion: 127
  • src/order-mcp/lib/order-mcp/orderMcpStack.ts:32Correct: nodeLayerVersion: 127

2. Incomplete Datadog Removal in Pricing Service

  • File: src/pricing-service/lib/pricing-api/pricingApiStack.ts:36
  • Issue: datadogConfiguration: undefined disables all monitoring for pricing service
  • Impact: Complete loss of observability, performance monitoring, and error tracking
  • Code Quality Issue: Comment on line 29 says "Paste Datadog configuration code here" indicating incomplete work
  • Recommendation: Either restore proper Datadog configuration or implement alternative monitoring

3. IAM Policy Duplicate Action

  • File: src/inventory-service/infra/modules/lambda-function/main.tf:39-40
  • Issue: Duplicate logs:CreateLogStream action in deny policy
  • Impact: Minor - doesn't affect functionality but indicates copy-paste error
  • Fix: Remove duplicate on line 40

Code Quality Assessment 📋

Positive Aspects:

  • ✅ Consistent extension layer version (85) across all services and deployment methods
  • ✅ Python layer versions correctly updated to 113 across all deployment methods
  • ✅ Node.js layer versions consistently updated to 127 in Terraform and CDK
  • ✅ Java layer version appropriately set to 24
  • ✅ .NET layer version appropriately set to 20
  • ✅ Deprecated DD_TRACE_REMOVE_INTEGRATION_SERVICE_NAMES_ENABLED successfully removed

Code Standards:

  • ✅ Proper Infrastructure-as-Code practices maintained
  • ✅ Environment variables follow consistent naming patterns
  • ✅ Resource naming conventions adhered to across services
  • ✅ Version parameterization properly implemented

Performance Considerations ⚡

  • Layer Updates: Newer versions typically include performance optimizations and bug fixes
  • No Breaking Changes: Version increments are within compatible ranges
  • ⚠️ Cold Start Impact: Extension layer updates may slightly affect cold start times, but benefits outweigh costs
  • ⚠️ Pricing Service: Disabling Datadog removes performance monitoring capabilities

Security Analysis 🔒

  • No Security Vulnerabilities: Layer updates include security patches
  • Secret Management: Proper handling of DD API keys via AWS Secrets Manager
  • IAM Permissions: Appropriate least-privilege access patterns maintained
  • ⚠️ Monitoring Gap: Pricing service loses security monitoring and anomaly detection

Test Coverage Assessment 🧪

  • Test Files Present: Unit and integration tests exist for affected services
  • Package.json Scripts: Test commands properly defined (test:unit, test:integration)
  • ⚠️ Layer Version Testing: No tests validate Datadog layer compatibility
  • Pricing Service Tests: May fail if they depend on Datadog functionality

Recommendations 📝

High Priority:

  1. Restore Datadog configuration in pricing service or implement alternative monitoring
  2. Fix duplicate IAM action in inventory service Terraform
  3. Test all services to ensure layer compatibility

Medium Priority:

  1. Add layer version validation to prevent incompatible combinations
  2. Update documentation reflecting the version changes
  3. Consider automated layer version updates for future maintenance

Overall Assessment

This is a maintenance PR that successfully updates Datadog layers across the microservices architecture. The majority of changes are well-executed and follow best practices. However, the incomplete Datadog removal in pricing service is a critical issue that must be addressed before merging.

Recommendation: ⚠️ Request changes - Fix the pricing service configuration and IAM policy duplicate before approving.


@jeastham1993 jeastham1993 merged commit 3062d66 into main Sep 1, 2025
14 of 28 checks passed
@jeastham1993 jeastham1993 deleted the feat/update-config branch September 1, 2025 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants