Chris/sch 3491 datastream in c

[cbrady]

No description provided.

[Copilot]

Pull Request Overview

Adds a new RulesEngine project to handle feature‐flag evaluation, metric periods, and entitlement logic, along with extensive model definitions and unit tests.

• Introduces domain models (Trait, Subscription, Rule, Flag, Condition, CompanyMetric, Company) with JSON serialization attributes
• Implements Metrics utility for calculating next reset dates and a FlagCheckService for evaluating flags against rules
• Adds NUnit test suites covering RuleCheckService and FlagCheckService, and updates solution and ignore files

Reviewed Changes

Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Trait.cs	Adds Trait model with JSON mappings
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Subscription.cs	Adds Subscription model
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Rule.cs	Defines Rule model including lists of conditions & groups
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Flag.cs	Defines Flag model with rules and default value
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/ConditionGroup.cs	Introduces ConditionGroup container
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Condition.cs	Defines Condition model covering various condition types
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/CompanyMetric.cs	Adds CompanyMetric model with lookup helper
src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Company.cs	Implements Company model with trait & metric management
src/SchematicHQ.RulesEngine/src/RulesEngine/Metrics.cs	Implements date‐calculation helpers for metric periods
src/SchematicHQ.RulesEngine/src/RulesEngine/FlagCheck.cs	Implements FlagCheckService and result types for flag logic
src/SchematicHQ.RulesEngine/src/RulesEngine/Consts.cs	Defines enums and extensions for rule/metric concepts
src/SchematicHQ.RulesEngine/src/RulesEngine/Class1.cs	Placeholder class added
src/SchematicHQ.RulesEngine/src/RulesEngine.Test/TestRuleCheck.cs	Unit tests for RuleCheckService
src/SchematicHQ.RulesEngine/src/RulesEngine.Test/TestFlagCheck.cs	Unit tests for FlagCheckService
src/SchematicHQ.RulesEngine/src/RulesEngine.Test/Helpers.cs	Test helper methods
src/SchematicHQ.RulesEngine/src/RulesEngine.Test/RulesEngine.Test.csproj	Adds test project configuration
src/SchematicHQ.Client.sln	Registers new projects and platform configurations
.fernignore	Updates ignore paths

Comments suppressed due to low confidence (4)

src/SchematicHQ.RulesEngine/src/RulesEngine/Models/Subscription.cs:1

• [nitpick] The Subscription model has been introduced but has no unit tests. Consider adding tests to verify its serialization behavior and any validation or business logic.

using System.Text.Json.Serialization;

src/SchematicHQ.RulesEngine/src/RulesEngine/Test/TestFlagCheck.cs:19

• The Errors class is defined in the SchematicHQ.RulesEngine namespace, but this test file doesn't import that namespace. Add 'using SchematicHQ.RulesEngine;' or fully qualify Errors.ErrorFlagNotFound so the reference compiles.

Assert.That(result.Error, Is.EqualTo(Errors.ErrorFlagNotFound));

.fernignore:17

• The path 'src/SchematicHQ.RunesEngine' is misspelled; it should be 'src/SchematicHQ.RulesEngine' to correctly ignore the RulesEngine folder.

src/SchematicHQ.RunesEngine

src/SchematicHQ.RulesEngine/src/RulesEngine/Class1.cs:3

• [nitpick] Class1.cs contains an empty placeholder class. Consider removing this file or replacing it with the intended implementation to avoid dead code.

public class Class1

[cbrady]

@claude review this PR

[claude]

Claude finished @cbrady's task in 1m 55s —— View job

---

PR Review Completed

This PR introduces significant new functionality with a datastream client and rules engine. Here's my comprehensive review:

Major Findings

🚨 Critical Issues

1. Typo in Constant - src/SchematicHQ.Client/Datastream/Client.cs:52

   private const int ResrouceTimeout = 2000; // Should be "ResourceTimeout"

   This is particularly problematic since it's defined but never used, while ResourceTimeout constant is used elsewhere.

2. Misspelled Path in .fernignore - Line 17

   src/SchematicHQ.RunesEngine  // Should be "RulesEngine"
   

3. Thread Safety Issues in Company.cs:60-76

   private Mutex mu = new Mutex(); // Should be readonly and properly disposed

   Using Mutex for thread safety in this context is heavyweight and could cause performance issues. Consider using lock statement or SemaphoreSlim.

⚠️ High Priority Issues

4. Memory Leak Risk - DatastreamClient.cs:166-167

   var random = new Random(); // Creates new Random instance unnecessarily

   Should use a static/shared Random instance or Random.Shared (in .NET 6+).

5. Resource Management - DatastreamClient.cs:834-856
   WebSocket disposal in Dispose() method has potential for hanging on closeTask.Wait(TimeSpan.FromSeconds(5)). Should use proper timeout handling.

6. Exception Handling - DatastreamClient.cs:202
   Console.WriteLine usage for error output instead of proper logging:

   Console.WriteLine($"Error checking rule {rule.Id}: {ex.Message}");
   Console.WriteLine(ex.StackTrace);

🔍 Architecture & Design Issues

7. Mixed Responsibilities - DatastreamClient.cs
   This class handles WebSocket connection, caching, message parsing, and business logic. Consider breaking into smaller, focused classes.

8. Cache Design Inconsistency - Cache.cs:76
   Flags cache uses TimeSpan.MaxValue (unlimited TTL) while other entities use configurable TTL. This could lead to stale flag data.

9. Type Safety - RulesEngine/Models/Condition.cs:38
   Previous review suggested using MetricPeriodMonthReset enum instead of int? for better type safety.

📋 Medium Priority Issues

10. Unused Code - Several constants defined but not used:

    • DatastreamClient.cs:52 - ResrouceTimeout (with typo)
    • DatastreamClient.cs:42-44 - Some timeout constants

11. Error Handling Inconsistency

    • Some methods return boolean for errors, others throw exceptions
    • Error messages sometimes use string constants, other times create new Exception instances

12. Logging Inconsistency

    • Mix of Debug, Info, Warn, and Error levels without clear guidelines
    • Some critical errors logged as warnings

Positive Aspects ✅

1. Excellent Test Coverage - Comprehensive test suite with good mocking strategies
2. Clean Separation - RulesEngine is properly separated from the main client
3. WebSocket Abstraction - Good interface design for testability
4. Reconnection Logic - Robust exponential backoff implementation
5. Concurrent Request Handling - Smart deduplication of pending requests

Recommendations

Immediate Actions (Blocking)

• Fix the typo: ResrouceTimeout → ResourceTimeout
• Fix .fernignore path: RunesEngine → RulesEngine
• Replace Mutex with appropriate synchronization primitive
• Fix console logging in FlagCheck.cs

Should Address Before Merge

• Implement proper resource disposal patterns
• Centralize error handling strategy
• Consider breaking down DatastreamClient into smaller classes
• Review caching strategy for consistency

Consider for Future

• Add metrics/telemetry for monitoring datastream performance
• Implement circuit breaker pattern for resilience
• Add comprehensive documentation for the new features

Overall Assessment

This is a substantial and well-implemented feature addition. The code demonstrates good understanding of async patterns, WebSocket handling, and testing practices. However, there are several issues that should be addressed, particularly the critical ones related to correctness and resource management.

Recommendation: Request changes to address the critical and high-priority issues before merging.

---

[cbrady]

@claude review this PR

[claude]

Claude finished @cbrady's task in 3m 28s —— View job

---

PR Review Complete ✅

I've conducted a comprehensive review of this substantial PR introducing datastream functionality and rules engine capabilities. Here's my detailed analysis:

🚨 Critical Issues

1. Inappropriate Console.WriteLine Usage - src/SchematicHQ.Client/Schematic.cs:193

catch (Exception ex)
{
    _logger.Error("Error checking flag via datastream: {0}", ex.Message);
    Console.WriteLine(ex.StackTrace); // ❌ Should use proper logging
    return GetFlagDefault(flagKey);
}

Fix: Replace with _logger.Error("Stack trace: {0}", ex.StackTrace); to maintain consistency with established logging infrastructure.

⚠️ Medium Priority Issues

2. Error Handling Inconsistency

• Some methods return boolean for errors, others throw exceptions
• Mix of error reporting strategies throughout the codebase
• Recommendation: Establish consistent error handling patterns

3. Large Class Responsibilities - DatastreamClient.cs

• Single class handles WebSocket connection, caching, message parsing, and business logic (~915 lines)
• Recommendation: Consider breaking into smaller, focused classes for better maintainability

4. Magic Number Usage

• Hard-coded timeouts and retry counts spread throughout
• Recommendation: Centralize configuration values

📋 Minor Issues

5. Code Organization

• Some constants defined but not used consistently
• Mixed casing in some variable names
• Recommendation: Code cleanup for consistency

6. Documentation

• Missing XML documentation for public APIs
• Complex logic could benefit from inline comments

✅ Excellent Aspects

1. Architecture & Design

• Clean Separation: Rules engine properly separated from main client
• Interface Design: Good abstractions for WebSocket and caching
• Dependency Injection: Proper use of constructor injection patterns

2. Thread Safety & Concurrency

• Request Deduplication: Smart handling of concurrent requests to same resource
• Thread-Safe Caching: Both local and Redis implementations are properly thread-safe
• Lock Usage: Proper use of locks with clear scope and avoiding deadlocks

3. Resource Management

• Disposal Patterns: Comprehensive IDisposable implementation in DatastreamClient
• Connection Management: Robust WebSocket connection handling with graceful shutdown
• Timeout Handling: Proper timeout management throughout

4. Resilience & Performance

• Exponential Backoff: Well-implemented reconnection logic with jitter
• Caching Strategy: Dual-tier caching (local + Redis) with configurable TTL
• LRU Eviction: Efficient cache management in LocalCache

5. Test Coverage

• Comprehensive Tests: Excellent test coverage including edge cases
• Mock Strategy: Clean mocking approach for WebSocket and external dependencies
• Concurrency Testing: Tests verify thread safety and race conditions

6. Configuration Flexibility

• Multiple Cache Providers: Support for both local and Redis caching
• Configurable Options: Extensive configuration through DatastreamOptions
• Environment Support: Proper handling of different deployment scenarios

🔧 Technical Implementation Highlights

WebSocket Management

• Robust connection handling with automatic reconnection
• Proper message framing and JSON serialization
• Clean resource cleanup on disposal

Rules Engine Design

• Type-safe condition evaluation
• Extensible rule types and prioritization
• Proper separation between rule checking and flag evaluation

Caching Implementation

• LRU eviction policy in LocalCache
• Redis failback to local cache
• Efficient batch operations for cache maintenance

📊 Impact Assessment

Scope: Large feature addition (6097 additions, 60 files)
Risk: Medium - Well-tested but complex functionality
Value: High - Enables real-time feature flag evaluation

🎯 Recommendations

Before Merge (Critical)

• Fix Console.WriteLine usage in Schematic.cs:193

Should Address (High Priority)

• Establish consistent error handling strategy
• Add XML documentation for public APIs
• Consider breaking down DatastreamClient into smaller classes

Consider for Future

• Add metrics/telemetry for monitoring
• Implement circuit breaker pattern
• Centralize configuration constants

🏆 Overall Assessment

This is a well-architected and thoroughly tested feature addition. The code demonstrates solid understanding of:

• Async/await patterns
• WebSocket handling
• Thread safety
• Resource management
• Caching strategies

The implementation shows careful consideration of edge cases, proper error handling (with one exception), and excellent test coverage.

Recommendation: Approve with minor changes - Fix the Console.WriteLine issue and this is ready to merge.

---

[bpapillon]

I think we may have missed this on the Go side as well, but we need to log flag checks from the client now that we're doing client-side evaluation. Similar to what's happening here for the browser JS client: https://github.com/SchematicHQ/schematic-js/blob/main/js/src/index.ts#L201

nit but I think we need to add some more files to .fernignore

[Copilot]

Pull Request Overview

This PR enhances and tests the datastream functionality and caching in the client library while updating project and CI configurations to support .NET 9.0 and new tooling. Key changes include:

• Adding and updating mock implementations and tests for WebSocket communication, concurrency, and caching.
• Upgrading project target frameworks and .NET GitHub Action versions.
• Introducing example applications for webhook and datastream test servers.

Reviewed Changes

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

Show a summary per file

File	Description
src/SchematicHQ.Client.Test/TestCache.cs	Adds a using directive for the cache namespace.
src/SchematicHQ.Client.Test/Datastream/Mocks/MockWebSocket.cs	Provides a full mock implementation of the WebSocket client with methods for setting up various response types.
src/SchematicHQ.Client.Test/Datastream/Mocks/MockSchematicLogger.cs	Introduces a logger mock for capturing log entries during tests.
src/SchematicHQ.Client.Test/Datastream/Mocks/DatastreamClientTestFactory.cs	Creates a test factory to assemble a datastream client along with its mocks.
src/SchematicHQ.Client.Test/Datastream/DatastreamConnectionTests.cs	Adds several tests verifying error handling, timeouts, and concurrent behaviors in datastream connections.
src/SchematicHQ.Client.Test/Datastream/DatastreamConcurrencyTests.cs	Implements concurrency tests ensuring that multiple requests reuse a single WebSocket request and handle warnings properly.
src/SchematicHQ.Client.Test/Datastream/DatastreamClientTests.cs	Provides extensive tests on flag handling, caching, and resource reuse via the datastream client.
src/SchematicHQ.Client.Test/Datastream/DatastreamCacheTests.cs	Tests the cache expiration logic to ensure resources are re-requested when the cache expires.
examples/webhook-test-server/.csproj, appsettings, Program.cs, etc.	Updates project configuration and example server files to align with the updated framework and tooling requirements.
.github/workflows/ci.yml	Upgrades CI configuration to use .NET 9.0 alongside .NET 8.0 and switches to setup-dotnet v3.
.fernignore	Adjusts file ignore patterns for Fern to include new project areas.

[Copilot]

Consider using 'await Task.Delay(100)' instead of blocking with .Wait() to align with asynchronous best practices and avoid potential deadlocks.

Suggested change

	Task.Delay(100).Wait();
	await Task.Delay(100);

[Copilot]

Replace 'Task.Delay(100).Wait()' with 'await Task.Delay(100)' for consistency with asynchronous patterns in tests.

Suggested change

	Task.Delay(100).Wait();
	await Task.Delay(100);

[bpapillon]

I think this is looking pretty good. Most of my concerns at this point are on the server side.