fix: loyalty point span tracking

[jeastham1993]

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

[claude]

Claude finished @jeastham1993's task —— View job

---

PR Review: Loyalty Point Span Tracking Fix

I've conducted a comprehensive review of this PR focusing on code quality, potential bugs, performance, security, and test coverage.

🔍 Summary of Changes

This PR implements improvements to loyalty point span tracking in the serverless sample application, adding support for v2 events and enhancing observability with proper span linking.

✅ Strengths

1. Proper Event Versioning Strategy

• Clean implementation of v2 event types (OrderCompletedEventV2, LoyaltyPointsAddedV2)
• Maintains backward compatibility by publishing both v1 and v2 events in eventBridgeEventPublisher.ts:42-85
• Includes deprecation date for v1 events (deprecationdate: new Date(2025, 11, 31)) - good practice

2. Enhanced Observability & Tracing

• Implements proper span linking with addLink() for distributed tracing (observability.ts:65)
• Comprehensive semantic conventions for messaging spans
• Good separation of concerns with dedicated EventBridgeEvent interface
• Proper error handling with span error tagging (eventBridgeEventPublisher.ts:104-114)

3. Test Coverage Improvements

• Added injectOrderCompletedV2Event() method in test driver for v2 event testing
• Maintains existing test patterns while extending coverage

🚨 Issues Found

1. Bug: Hardcoded Points Difference

// handleLoyaltyPointsUpdated.ts:73
difference: 50,  // This should be calculated, not hardcoded!

This hardcoded value doesn't reflect actual point changes and will always report 50 regardless of the actual difference.

2. Type Safety Issues

// handleOrderCompletedFunction.ts:49
const evtWrapper: EventBridgeEvent<CloudEvent<any>> = JSON.parse(message.body);

Using any defeats the purpose of TypeScript's type safety. Should be properly typed.

3. Potential Memory Issues

// handleLoyaltyPointsUpdated.ts:65-67
const loyaltyAccount = new LoyaltyPoints(
  loyaltyData["PK"].S!,  // Direct property access without null checks
  parseFloat(loyaltyData["Points"].N!), 
  JSON.parse(loyaltyData["Orders"].S!)  // Potential JSON parsing errors
);

4. Error Handling Inconsistencies

• handleLoyaltyPointsUpdated.ts:78 throws errors that could cause Lambda failures
• Missing validation for DynamoDB record structure before processing

⚠️ Security Concerns

1. Input Validation

• Missing validation of JSON parsing operations
• No sanitization of user input in event processing
• Direct property access without existence checks

🔧 Recommendations

1. Fix Points Difference Calculation

// Calculate actual difference instead of hardcoding
const oldPoints = record.dynamodb?.OldImage ? 
  parseFloat(record.dynamodb.OldImage["Points"].N!) : 0;
const newPoints = parseFloat(loyaltyData["Points"].N!);
const actualDifference = newPoints - oldPoints;

await eventPublisher.publishLoyaltyPointsUpdated({
  userId: loyaltyAccount.userId,
  totalPoints: loyaltyAccount.currentPoints,
  difference: actualDifference, // Use calculated value
});

2. Improve Type Safety

interface OrderCompletedCloudEvent extends CloudEvent<OrderCompletedEventV1 | OrderCompletedEventV2> {}
const evtWrapper: EventBridgeEvent<OrderCompletedCloudEvent> = JSON.parse(message.body);

3. Add Proper Error Boundaries

try {
  const loyaltyData = DynamoDB.Converter.unmarshall(record.dynamodb.NewImage);
  
  // Validate required fields exist
  if (!loyaltyData.PK?.S || !loyaltyData.Points?.N) {
    logger.warn("Missing required fields in DynamoDB record");
    return;
  }
  
  // Continue processing...
} catch (error) {
  logger.error("Failed to process loyalty points record", { error, recordId: record.eventID });
  // Don't rethrow - let other records process
  return;
}

📊 Performance Analysis

Positive:

• Efficient batch processing in SQS handlers
• Proper span lifecycle management
• Good use of AWS SDK v3

Areas for Improvement:

• Large package-lock.json updates (13K+ additions) - consider dependency audit
• JSON parsing in hot path could benefit from validation caching

🧪 Test Coverage Assessment

Good:

• Added v2 event testing capabilities
• Maintains existing integration test patterns

Missing:

• No unit tests for the hardcoded difference bug
• Missing error case testing for malformed DynamoDB records
• No tests for span linking functionality

📈 Overall Assessment

This PR shows good architectural thinking with proper event versioning and observability improvements. However, it contains a significant functional bug (hardcoded points difference) and some type safety issues that should be addressed before merging.

Recommendation: Request Changes - Fix the hardcoded difference calculation and improve error handling before merging.

---

[Copilot]

Pull Request Overview

This PR fixes loyalty point span tracking by adding support for v2 order completed events while maintaining backward compatibility with v1 events. The changes ensure proper event processing and observability for both event versions.

• Adds support for processing both v1 and v2 order completed events in the loyalty service
• Updates event publishing to emit both v1 and v2 events with deprecation handling
• Fixes observability configuration by correcting the messaging system type from "sns" to "sqs"

Reviewed Changes

Copilot reviewed 16 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
end-to-end.test.ts	Adds comprehensive test coverage for v2 events and dual event processing scenarios
apiDriver.ts	Implements new method to inject v2 order completed events for testing
observability.ts	Corrects messaging system type from "sns" to "sqs" for proper span tracking
orderCompletedEventV2.ts	Defines new v2 event interface with orderId instead of orderNumber
loyaltyPointsUpdatedV2.ts	Defines new v2 loyalty points updated event interface
eventPublisher.ts	Updates interface to use v2 event types
handleOrderCompletedFunction.ts	Adds logic to process both v1 and v2 events with proper error handling
eventBridgeEventPublisher.ts	Implements dual event publishing for backward compatibility
dynamoDbLoyaltyPointRepository.ts	Updates constructor to accept logger parameter
loyaltyApiStack.ts	Updates Datadog extension layer version
loyaltyAcl.ts	Adds v2 event subscription and comments out propagation settings

Files not reviewed (1)

• src/user-management-service/package-lock.json: Language not supported

---

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

[Copilot]

The span finishing logic is incorrectly placed. The span should be finished in a finally block to ensure it's always closed, even when exceptions occur. The current code will leave spans open when errors happen.

Suggested change

	}
	messageProcessingSpan?.finish();
	} finally {
	messageProcessingSpan?.finish();
	}

[Copilot]

The Date constructor month parameter is 0-indexed, so 11 represents December, but this creates a date in 2025 which is in the past (current date is October 2025). This should likely be new Date(2026, 11, 31) for December 31st, 2026.

Suggested change

	deprecationdate: new Date(2025, 11, 31).toISOString(),
	deprecationdate: new Date(2026, 11, 31).toISOString(),

[Copilot]

Missing comma after the traceparent property. While JavaScript allows this, it's inconsistent with the coding style used elsewhere in the object and can cause issues when adding new properties.

Suggested change

	traceparent: parentSpan?.context().toTraceparent()
	traceparent: parentSpan?.context().toTraceparent(),

[Copilot]

The removal of pre-commit from the .PHONY list is inconsistent with its removal from other parts of the Makefile. Since pre-commit functionality has been removed throughout the file, this reference should also be removed for consistency.