Feat/phase4 fe features - #18
Merged
Merged
Conversation
…uction and constraints
ravin00
requested review from
Dinuda
and
a lite review from Copilot
and removed request for
Copilot
August 5, 2026 11:54
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe project adds feature-group computation for a 47-dimensional state vector, state-vector assembly and sanitization, dependency-injection wiring, and HTTP endpoints with integration tests. ChangesFeature engineering state flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StateEndpoints
participant WindowStore
participant StateVectorBuilder
Client->>StateEndpoints: GET workload state
StateEndpoints->>WindowStore: Snapshot workload samples
WindowStore-->>StateEndpoints: Samples or untracked workload
StateEndpoints->>StateVectorBuilder: Build workload state vector
StateVectorBuilder-->>StateEndpoints: StateVector with 47 values
StateEndpoints-->>Client: State response or 404
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/feature-engineering/appsettings.json`:
- Line 22: Align the blob storage configuration key with the existing
EventHubOptions contract by renaming the option from BlobStorageUrl to
BlobStorageUri, or restore the JSON key to BlobStorageUrl; ensure
EventHubConsumer and any related configuration binding use the same key
consistently.
In `@src/api/feature-engineering/Features/ActionHistoryFeatures.cs`:
- Around line 16-19: Update the minutesScaled calculation in the action-history
feature encoding to clamp elapsed time at a minimum of 0 as well as the existing
maximum of 1. Ensure future record.Timestamp values encode zero elapsed time
while preserving the current [0, 1] range for normal timestamps.
In `@src/api/feature-engineering/Features/NodePressureFeatures.cs`:
- Around line 19-21: Update the utilization assignment in the feature
computation to clamp the maximum of cpuUtil and memUtil to the inclusive range
[0, 1], preserving the existing upper-bound behavior while preventing negative
values from reaching dest[2].
In `@src/api/feature-engineering/Features/TemporalFeatures.cs`:
- Around line 22-25: Update the age calculation in the temporal feature
computation to use the latest sample timestamp via ctx.Samples[^1].Timestamp
rather than the first sample. Clamp the computed age to the range [0, 365]
before dividing by 365 and assigning dest[4], while preserving the zero value
for empty samples.
In `@src/api/feature-engineering/Models/StateVector.cs`:
- Around line 3-10: Update the public StateVector constructor to reject null
Values and any array whose length differs from Dimensions (47), while preserving
valid construction and serialization for correctly sized vectors.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4c2b7431-79f9-443b-a577-81bef654da6b
📒 Files selected for processing (24)
src/api/feature-engineering/AFIE.FeatureEngineering.csprojsrc/api/feature-engineering/Endpoints/StateEndpoints.cssrc/api/feature-engineering/Features/ActionHistoryFeatures.cssrc/api/feature-engineering/Features/AppSignalFeature.cssrc/api/feature-engineering/Features/CostFeatures.cssrc/api/feature-engineering/Features/CpuFeatures.cssrc/api/feature-engineering/Features/DeploymentFeatures.cssrc/api/feature-engineering/Features/IFeatureGroup.cssrc/api/feature-engineering/Features/MemoryFeatures.cssrc/api/feature-engineering/Features/NodePressureFeatures.cssrc/api/feature-engineering/Features/StateVectorBuilder.cssrc/api/feature-engineering/Features/TemporalFeatures.cssrc/api/feature-engineering/Models/FeatureEngineeringOptions.cssrc/api/feature-engineering/Models/StateVector.cssrc/api/feature-engineering/Program.cssrc/api/feature-engineering/appsettings.jsontests/AFIE.FeatureEngineering.Tests/AFIE.FeatureEngineering.Tests.csprojtests/AFIE.FeatureEngineering.Tests/Endpoints/StateEndpointsTests.cstests/AFIE.FeatureEngineering.Tests/Features/ActionHistoryFeaturesTests.cstests/AFIE.FeatureEngineering.Tests/Features/CpuFeaturesTests.cstests/AFIE.FeatureEngineering.Tests/Features/MemoryFeaturesTests.cstests/AFIE.FeatureEngineering.Tests/Features/NodePressureFeaturesTests.cstests/AFIE.FeatureEngineering.Tests/Features/StateVectorBuilderTests.cstests/AFIE.FeatureEngineering.Tests/Features/TemporalFeaturesTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
Adds the Phase 4 feature-engineering “state vector” pipeline by composing multiple IFeatureGroups into a fixed 47-dim observation and exposing it via minimal-API endpoints for per-workload retrieval.
Changes:
- Introduces 8 feature groups +
StateVectorBuilderto compute a clamped/finite 47-dimensional state vector. - Adds
/state/{workloadName}and/state/{workloadName}/latestendpoints to retrieve current vectors from the sliding window store. - Adds unit/integration tests and brings in MathNet + MVC testing dependencies.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/AFIE.FeatureEngineering.Tests/Features/TemporalFeaturesTests.cs | Verifies temporal sine/cosine continuity around boundaries. |
| tests/AFIE.FeatureEngineering.Tests/Features/StateVectorBuilderTests.cs | Validates builder dimension coverage, clamping, and finiteness. |
| tests/AFIE.FeatureEngineering.Tests/Features/NodePressureFeaturesTests.cs | Tests node-pressure flag/util feature behavior. |
| tests/AFIE.FeatureEngineering.Tests/Features/MemoryFeaturesTests.cs | Tests memory percentile features at limit. |
| tests/AFIE.FeatureEngineering.Tests/Features/CpuFeaturesTests.cs | Tests CPU percentile features and empty/short sample handling. |
| tests/AFIE.FeatureEngineering.Tests/Features/ActionHistoryFeaturesTests.cs | Tests action-history encoding into the vector tail. |
| tests/AFIE.FeatureEngineering.Tests/Endpoints/StateEndpointsTests.cs | Integration coverage for the new /state/* endpoints. |
| tests/AFIE.FeatureEngineering.Tests/AFIE.FeatureEngineering.Tests.csproj | Adds Microsoft.AspNetCore.Mvc.Testing for endpoint tests. |
| src/api/feature-engineering/Program.cs | Registers feature groups/builder and maps state endpoints. |
| src/api/feature-engineering/Models/StateVector.cs | Adds the StateVector model and dimension constant. |
| src/api/feature-engineering/Models/FeatureEngineeringOptions.cs | Adds deploy-time overridable cost/budget coefficients. |
| src/api/feature-engineering/Features/TemporalFeatures.cs | Implements temporal encodings (hour/day-of-week + age). |
| src/api/feature-engineering/Features/StateVectorBuilder.cs | Orchestrates group computation and post-processing (finite + clamp). |
| src/api/feature-engineering/Features/NodePressureFeatures.cs | Implements node pressure flags + combined utilization. |
| src/api/feature-engineering/Features/MemoryFeatures.cs | Implements memory usage/limit percentile features. |
| src/api/feature-engineering/Features/IFeatureGroup.cs | Defines the feature-group contract and shared FeatureContext. |
| src/api/feature-engineering/Features/DeploymentFeatures.cs | Adds placeholder deployment features for future phases. |
| src/api/feature-engineering/Features/CpuFeatures.cs | Implements CPU usage/limit percentile features and slice helper. |
| src/api/feature-engineering/Features/CostFeatures.cs | Adds hourly-cost and budget-relative cost features (plus placeholder trend). |
| src/api/feature-engineering/Features/AppSignalFeature.cs | Adds app-signal aggregation features (with one reserved dim). |
| src/api/feature-engineering/Features/ActionHistoryFeatures.cs | Encodes last few actions into fixed slots. |
| src/api/feature-engineering/Endpoints/StateEndpoints.cs | Implements /state/{workloadName} and /latest endpoints. |
| src/api/feature-engineering/appsettings.json | Adds FeatureEngineering cost settings and modifies EventHub config key. |
| src/api/feature-engineering/AFIE.FeatureEngineering.csproj | Adds MathNet dependency for percentile computation. |
Owner
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/api/feature-engineering/Models/StateVector.cs`:
- Line 11: Update the StateVector Values property to use a backing field and
validating init accessor so constructor assignments, object initializers, and
with expressions all reject arrays whose length is not 47; preserve the existing
ValidateValues behavior for valid arrays, and add a regression test covering
invalid assignment through an object initializer or with expression.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ecd8653e-40e4-42dc-a27e-2127258c88e2
📒 Files selected for processing (3)
src/api/feature-engineering/Features/TemporalFeatures.cssrc/api/feature-engineering/Models/StateVector.cstests/AFIE.FeatureEngineering.Tests/Features/StateVectorBuilderTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/feature-engineering/Features/TemporalFeatures.cs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/AFIE.FeatureEngineering.Tests/Models/StateVectorTests.cs (1)
11-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for constructor validation.
StateVectorvalidatesValuesduring primary construction and in theinitaccessor. These tests cover onlywithand object-initializer paths. A constructor regression could therefore pass this test file. Add constructor cases fornulland invalid lengths. Include a length greater thanStateVector.Dimensions.Suggested additions
+ [Fact] + public void Constructor_WrongLength_Throws() + { + Assert.Throws<ArgumentException>(() => + new StateVector("nginx", "default", DateTimeOffset.UtcNow, + new float[StateVector.Dimensions + 1])); + } + + [Fact] + public void Constructor_NullValues_Throws() + { + Assert.Throws<ArgumentNullException>(() => + new StateVector("nginx", "default", DateTimeOffset.UtcNow, null!)); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/AFIE.FeatureEngineering.Tests/Models/StateVectorTests.cs` around lines 11 - 33, Add direct constructor-validation tests for StateVector, covering null Values, a length below StateVector.Dimensions, and a length greater than StateVector.Dimensions. Use the constructor path rather than with expressions or object initializers, and assert ArgumentNullException for null and ArgumentException for invalid lengths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/AFIE.FeatureEngineering.Tests/Models/StateVectorTests.cs`:
- Around line 11-33: Add direct constructor-validation tests for StateVector,
covering null Values, a length below StateVector.Dimensions, and a length
greater than StateVector.Dimensions. Use the constructor path rather than with
expressions or object initializers, and assert ArgumentNullException for null
and ArgumentException for invalid lengths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 876e5ace-5a2b-4cee-82e7-f6900506c53b
📒 Files selected for processing (2)
src/api/feature-engineering/Models/StateVector.cstests/AFIE.FeatureEngineering.Tests/Models/StateVectorTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/api/feature-engineering/Models/StateVector.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds the 47-dimensional state-vector computation and the
/state/{workloadName}HTTP endpoint. This closes Phase 4's core loop: JSONL events from Phase 3
now feed a per-workload sliding window that gets projected into the fixed
observation shape the RL agent (Phase 5) will consume.
IFeatureGroupcomposition with 8 concrete groups covering dims 0–46:Cpu, Memory, AppSignal, NodePressure, Cost, Temporal, Deployment,
ActionHistory.
StateVectorBuildervalidates that groups cover[0, 47)contiguouslyat construction time, dispatches each group to its span slice, then
clamps
[-2, 2]and coercesNaN/±Inf→ 0.StateVectorrecord withDimensions = 47invariant.GET /state/{workloadName}returns 404 for unknownworkloads or 200 with a
float[47]JSON body. Companion/state/{workloadName}/latestreturns the same values with thetimestamp for debugging.
ConfiguredBudgetUsdPerHour,CpuCostPerCoreHourUsd,MemCostPerGiBHourUsd) added toFeatureEngineeringOptionsandappsettings.jsonso they're overridable at deploy time.MathNet.Numerics5.0 for percentile computation;Microsoft.AspNetCore.Mvc.Testing8.0.11 for the endpoint integrationtest.
Design decisions
{P50, P95, P99} × {5m, 15m, 1h}ofusage / limit.CpuLimit/MemLimitact as normalisation denominators, not standalone dims — thisis what makes the 47-total add up.
6-dim group; keeping the slot at zero preserves indices for a future
signal without shifting the vector.
cost 7-day-trend (28) return zeros or fixed placeholders. Real values
come from Phase 6 (K8s informer) and Phase 8 (Cosmos DB trend store) —
documented as such in code.
IFeatureGroupis usedcompositionally (all 8 groups always run), unlike
IMetricEventConsumerand
IStateVectorPublisherwhich are config-switched strategies.Test plan
dotnet build AFIE.slnx— cleandotnet test AFIE.slnx— 52 passing (20 telemetry + 32 FE)WebApplicationFactory-backed endpoint test:/state/missingreturns 404,
/state/nginx-endpoint-testreturns 47-length arrayday-of-week-adjacent boundaries within 0.01 of each other
leave a gap
dotnet run+ append MetricEvent to today's JSONL,confirm
curl /state/nginx | jq 'length'returns 47 and allvalues fall in
[-1, 1]Summary by CodeRabbit