Feat: Telemetry #131
Conversation
|
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:
📝 WalkthroughWalkthroughAdds structured rebalance event telemetry ( ChangesRebalance telemetry and storage query pipeline
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🤖 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/YoloApp/Commands/RebalanceCommand.cs`:
- Around line 227-228: The `RebalanceCommand` update mapping is treating every
non-null message as an error by falling back to `update.Message` in `Error`.
Update the object creation in `RebalanceCommand` so `Error` is only set when the
update represents a failure state (for example, based on `update.Type`), and
leave it null for successful or informational updates. Keep `Status =
update.Type.ToString()` unchanged and use the `update.Type`/`update.Error`
fields to decide whether `Error` should be populated.
- Around line 140-149: Telemetry recording in RebalanceCommand.ExecuteAsync
should not be able to stop the main order-processing loop. Wrap the
_tradeExecutionRecorder.RecordAsync call in a local try/catch inside the
TryBuildTradeExecutionRecord block, log any recorder exception, and continue
processing subsequent updates; make sure OperationCanceledException still
propagates so cooperative cancellation is preserved.
In `@src/YoloFunk/Infrastructure/RawJsonPersistenceHandler.cs`:
- Around line 102-110: The hash key generation in RawJsonPersistenceHandler is
ambiguous because string.Join on method, URI, bodies, and status can collide
when request or response text contains newlines. Update the input construction
in the hashing logic to use an unambiguous, delimiter-safe
encoding/serialization before SHA256.HashData, so distinct exchanges always
produce distinct pre-hash strings. Keep the fix within the hash-building path
around the input variable and the GetHash/identifier generation flow used for
storage keys.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c8c56f2-9a6a-407a-a77e-dddc35c94c7e
📒 Files selected for processing (8)
src/YoloAbstractions/Interfaces/ITradeExecutionRecorder.cssrc/YoloAbstractions/NoOpTradeExecutionRecorder.cssrc/YoloAbstractions/TradeExecutionRecord.cssrc/YoloApp/Commands/RebalanceCommand.cssrc/YoloFunk/Extensions/AddBlobStorageServices.cssrc/YoloFunk/Extensions/AddStrategyServices.cssrc/YoloFunk/Infrastructure/AzureTableTradeExecutionRecorder.cssrc/YoloFunk/Infrastructure/RawJsonPersistenceHandler.cs
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/YoloFunk/Functions/StorageQueryFunctions.cs`:
- Around line 148-158: Split the shared client lookup in TryGetStorageClients so
endpoints no longer fail unless both storage clients exist. Refactor the current
table/blob resolution into separate helpers or optional retrieval paths for
TableServiceClient and BlobServiceClient, then update the callers in
StorageQueryFunctions so table-only routes require only the table client and
blob-only routes require only the blob client. Keep the existing
serviceProvider/FunctionContext.InstanceServices fallback logic, but make each
endpoint validate only the client it actually uses.
- Around line 173-183: The query in StorageQueryFunctions is materializing all
matching trade execution entities in memory before paging, so a single page
request still scales with the full result set. Update the
TradeExecutionsTableName path and its
QueryEntitiesAsync/ToTradeExecutionQueryItem pipeline to fetch only one page
using storage pagination (for example AsPages with continuation tokens) or
another bounded query approach, and apply the page limit before any in-memory
sorting/filtering so the endpoint returns just one page without loading
everything first.
In `@src/YoloFunk/Infrastructure/HttpQueryParameters.cs`:
- Around line 56-59: The GetDateTimeOffset method currently relies on
DateTimeOffset.TryParse, which can vary by host culture and may infer local time
zones for ambiguous inputs. Update this parsing path to use an invariant
approach, such as TryParseExact with CultureInfo.InvariantCulture, or explicitly
reject ambiguous date formats for the from/to query parameters. Keep the change
localized to HttpQueryParameters.GetDateTimeOffset so API query parsing is
deterministic across environments.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c8cb4a7-556c-4a4d-ab37-28fc3a77c307
📒 Files selected for processing (7)
src/YoloApp/Commands/RebalanceCommand.cssrc/YoloFunk/Dto/HttpRequestCaptureQueryItem.cssrc/YoloFunk/Dto/PagedQueryResponse.cssrc/YoloFunk/Dto/TradeExecutionQueryItem.cssrc/YoloFunk/Functions/StorageQueryFunctions.cssrc/YoloFunk/Infrastructure/HttpQueryParameters.cssrc/YoloFunk/Infrastructure/RawJsonPersistenceHandler.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/YoloFunk/Infrastructure/RawJsonPersistenceHandler.cs
- src/YoloApp/Commands/RebalanceCommand.cs
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
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 `@test/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cs`:
- Around line 117-129: The test for StorageQueryFunctions.GetTradeExecutions is
using a date-like `from` value that some parsers may still accept, which makes
the `!from.HasValue` branch unreliable. Update the request in the
GetTradeExecutions test to use an unmistakably invalid `from` value so the date
parsing fails consistently and the code path that skips the date filter in
StorageQueryFunctions is exercised deterministically.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a67ebdf-6b5c-4991-9875-a19a7d78f753
📒 Files selected for processing (5)
src/YoloFunk/Dto/PagedQueryResponse.cssrc/YoloFunk/Functions/StorageQueryFunctions.cssrc/YoloFunk/Infrastructure/HttpQueryParameters.cstest/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cstest/YoloFunk.Test/Infrastructure/AzureTableTradeExecutionRecorderTest.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/YoloFunk/Dto/PagedQueryResponse.cs
- src/YoloFunk/Infrastructure/HttpQueryParameters.cs
- src/YoloFunk/Functions/StorageQueryFunctions.cs
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/YoloApp/Commands/RebalanceCommand.cs (2)
242-321: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEmit
RunFailedfor setup-phase exceptions too.The
try/catchat Lines 242-321 only coversManageOrdersAsync. Any exception from open-order cancellation, position/weight/market fetches, or trade planning exits afterRunStartedwithout a terminal failure event, so the query side sees an orphaned run instead of a failed one. Move the failure-recording catch around the full execution path afterrunIdis created, while keeping the current cancellation exceptions exempted.🤖 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 `@src/YoloApp/Commands/RebalanceCommand.cs` around lines 242 - 321, The current failure handling around RebalanceCommand’s order-management flow only wraps ManageOrdersAsync, so setup-phase exceptions can bypass the terminal RunFailed event and leave an orphaned run. Expand the existing try/catch in RebalanceCommand so it covers the full execution path after runId is created, including open-order cancellation, position/weight/market fetches, and trade planning, while preserving the current special handling for ChannelClosedException and OperationCanceledException. Keep the existing RecordRebalanceEventAsync RunFailed call in the outer catch and ensure it is reached for any unexpected exception outside the cancellation cases.
281-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle recorder
OperationCanceledExceptionselectively
Azure.Data.Tablescan surface request/network timeouts asOperationCanceledException, so rethrowing every one here will make recorder failures look like caller cancellation and can abort the rebalance. Only rethrow whencancellationToken.IsCancellationRequested; otherwise log and continue like the other telemetry failures.🤖 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 `@src/YoloApp/Commands/RebalanceCommand.cs` around lines 281 - 283, The `catch (OperationCanceledException)` in `RebalanceCommand` is treating all cancellations as caller-driven, which can incorrectly abort rebalance when `Azure.Data.Tables` raises timeout/network failures as `OperationCanceledException`. Update the `RebalanceCommand` exception handling so it only rethrows when `cancellationToken.IsCancellationRequested`; otherwise handle it the same way as the other recorder/telemetry failures by logging the error and continuing. Keep the change localized to the recorder handling path in `RebalanceCommand`.src/YoloFunk/Functions/StorageQueryFunctions.cs (2)
41-58: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrade-execution pagination no longer matches the requested ordering.
QueryEntitiesPageAsyncpicks the next slice in table order, and only thenApplyTradeExecutionSortreorders that slice. WithorderBy=coin/status/etc., later continuation-token pages can contain items that should sort ahead of earlier pages, andpageis only reflected in metadata becauseskipItems: falseskips no items. This route needs to either enforce storage-order pagination only or make the cursor depend on the requested sort.Also applies to: 224-256, 362-379, 426-447
🤖 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 `@src/YoloFunk/Functions/StorageQueryFunctions.cs` around lines 41 - 58, The trade-execution paging flow in StorageQueryFunctions is sorting only after QueryTradeExecutionsAsync returns a storage-ordered slice, so continuation tokens and page boundaries no longer respect the requested orderBy. Update the paging logic used by the affected query methods to either restrict pagination to the underlying table order or make the continuation cursor/slice selection in QueryTradeExecutionsAsync and WritePagedResponseAsync depend on the requested sort field, so later pages cannot miss or reorder items relative to earlier pages.
259-315: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
http-requestsandrebalance-eventsstill page after loading everything.Both helpers call
QueryEntitiesAsync, which drains the full Azure Table query into memory and only then sorts andSkip/Takes inWritePagedResponseAsync. On growing telemetry tables, one page request will scale with the entire match set instead ofpageSize. These routes need the same bounded page retrieval/continuation-token approach as the trade-execution path.Also applies to: 318-333, 426-447
🤖 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 `@src/YoloFunk/Functions/StorageQueryFunctions.cs` around lines 259 - 315, The `QueryHttpRequestCapturesAsync` and `QueryRebalanceEventsAsync` helpers still load the full result set via `QueryEntitiesAsync` before paging, so the `http-requests` and `rebalance-events` routes scale with total matches instead of `pageSize`. Update these query paths to use the same bounded retrieval/continuation-token pattern already used by the trade-execution flow, and wire the pagination through `WritePagedResponseAsync` so only one page of Azure Table entities is fetched and returned at a time.
🤖 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/YoloFunk/Infrastructure/AzureTableRebalanceEventRecorder.cs`:
- Around line 16-17: The table initialization in RecordAsync is on the hot path
because AzureTableRebalanceEventRecorder creates the table on every event. Move
the CreateIfNotExistsAsync call out of RecordAsync and initialize the table
once, either in the constructor or via a cached lazy initialization helper on
AzureTableRebalanceEventRecorder using the existing TableName/TableServiceClient
setup. Keep RecordAsync focused on writing records after initialization has
already happened.
---
Outside diff comments:
In `@src/YoloApp/Commands/RebalanceCommand.cs`:
- Around line 242-321: The current failure handling around RebalanceCommand’s
order-management flow only wraps ManageOrdersAsync, so setup-phase exceptions
can bypass the terminal RunFailed event and leave an orphaned run. Expand the
existing try/catch in RebalanceCommand so it covers the full execution path
after runId is created, including open-order cancellation,
position/weight/market fetches, and trade planning, while preserving the current
special handling for ChannelClosedException and OperationCanceledException. Keep
the existing RecordRebalanceEventAsync RunFailed call in the outer catch and
ensure it is reached for any unexpected exception outside the cancellation
cases.
- Around line 281-283: The `catch (OperationCanceledException)` in
`RebalanceCommand` is treating all cancellations as caller-driven, which can
incorrectly abort rebalance when `Azure.Data.Tables` raises timeout/network
failures as `OperationCanceledException`. Update the `RebalanceCommand`
exception handling so it only rethrows when
`cancellationToken.IsCancellationRequested`; otherwise handle it the same way as
the other recorder/telemetry failures by logging the error and continuing. Keep
the change localized to the recorder handling path in `RebalanceCommand`.
In `@src/YoloFunk/Functions/StorageQueryFunctions.cs`:
- Around line 41-58: The trade-execution paging flow in StorageQueryFunctions is
sorting only after QueryTradeExecutionsAsync returns a storage-ordered slice, so
continuation tokens and page boundaries no longer respect the requested orderBy.
Update the paging logic used by the affected query methods to either restrict
pagination to the underlying table order or make the continuation cursor/slice
selection in QueryTradeExecutionsAsync and WritePagedResponseAsync depend on the
requested sort field, so later pages cannot miss or reorder items relative to
earlier pages.
- Around line 259-315: The `QueryHttpRequestCapturesAsync` and
`QueryRebalanceEventsAsync` helpers still load the full result set via
`QueryEntitiesAsync` before paging, so the `http-requests` and
`rebalance-events` routes scale with total matches instead of `pageSize`. Update
these query paths to use the same bounded retrieval/continuation-token pattern
already used by the trade-execution flow, and wire the pagination through
`WritePagedResponseAsync` so only one page of Azure Table entities is fetched
and returned at a time.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f98d853-4d19-4a84-825e-4fad5fe26d82
📒 Files selected for processing (13)
src/YoloAbstractions/Interfaces/IRebalanceEventRecorder.cssrc/YoloAbstractions/NoOpRebalanceEventRecorder.cssrc/YoloAbstractions/RebalanceEventRecord.cssrc/YoloApp/Commands/RebalanceCommand.cssrc/YoloFunk/Dto/RebalanceEventQueryItem.cssrc/YoloFunk/Extensions/AddBlobStorageServices.cssrc/YoloFunk/Extensions/AddStrategyServices.cssrc/YoloFunk/Functions/StorageQueryFunctions.cssrc/YoloFunk/Infrastructure/AzureTableRebalanceEventRecorder.cstest/YoloApp.Test/Commands/RebalanceCommandTest.cstest/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cstest/YoloFunk.Test/Infrastructure/AzureTableRebalanceEventRecorderTest.cstest/YoloFunk.Test/Infrastructure/RawJsonPersistenceHandlerTest.cs
✅ Files skipped from review due to trivial changes (2)
- src/YoloAbstractions/Interfaces/IRebalanceEventRecorder.cs
- src/YoloAbstractions/NoOpRebalanceEventRecorder.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/YoloFunk/Extensions/AddBlobStorageServices.cs
- src/YoloFunk/Extensions/AddStrategyServices.cs
- test/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/YoloFunk/Functions/StorageQueryFunctions.cs (1)
49-58: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
pageis ignored for table queries without a continuation token.Both endpoints always write the first storage page with
skipItems: false, so?page=2still returns page 1's rows and just echoesPage = 2. Either preserve offset paging whencontinuationTokenis absent, or rejectpage > 1for these routes.Also applies to: 105-114, 306-327
🤖 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 `@src/YoloFunk/Functions/StorageQueryFunctions.cs` around lines 49 - 58, The table-query endpoints in StorageQueryFunctions are ignoring the requested page when no continuation token is provided because WritePagedResponseAsync is always called with skipItems: false. Update the logic in the affected query handlers to either apply offset paging from the page parameter in the no-continuation-token path or explicitly reject page values greater than 1, and make the behavior consistent across the methods that call WritePagedResponseAsync and the shared response-writing flow.
🤖 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/YoloFunk/Functions/StorageQueryFunctions.cs`:
- Around line 216-234: The paging logic in the query helpers is applying filters
only after QueryEntitiesPageAsync returns a raw page, which can under-fill
results and advance the continuation token past valid matches. Update the
pagination flow in StorageQueryFunctions so it keeps fetching additional table
pages until you accumulate pageSize filtered items or run out of data, rather
than finalizing after the first raw page; make the same fix in the other
affected query methods that use the same pattern. Ensure the returned
continuation token and any count metadata reflect the fully filtered page
window, not just the first slice.
---
Outside diff comments:
In `@src/YoloFunk/Functions/StorageQueryFunctions.cs`:
- Around line 49-58: The table-query endpoints in StorageQueryFunctions are
ignoring the requested page when no continuation token is provided because
WritePagedResponseAsync is always called with skipItems: false. Update the logic
in the affected query handlers to either apply offset paging from the page
parameter in the no-continuation-token path or explicitly reject page values
greater than 1, and make the behavior consistent across the methods that call
WritePagedResponseAsync and the shared response-writing flow.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 27c5b1f2-34f4-42d8-9765-c11ebf447f97
📒 Files selected for processing (7)
src/YoloApp/Commands/RebalanceCommand.cssrc/YoloFunk/Extensions/AddBlobStorageServices.cssrc/YoloFunk/Extensions/AddStrategyServices.cssrc/YoloFunk/Functions/StorageQueryFunctions.cssrc/YoloFunk/Infrastructure/AzureTableRebalanceEventRecorder.cstest/YoloApp.Test/Commands/RebalanceCommandTest.cstest/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cs
💤 Files with no reviewable changes (3)
- src/YoloFunk/Extensions/AddBlobStorageServices.cs
- test/YoloFunk.Test/Functions/StorageQueryFunctionsTest.cs
- src/YoloFunk/Extensions/AddStrategyServices.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/YoloFunk/Infrastructure/AzureTableRebalanceEventRecorder.cs
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
🚀 Deployed to Azure FunctionsEnvironment: This environment will be automatically cleaned up when the PR is closed. |
1 participant
Record provider web service JSON, as well as trade execution data
Summary by CodeRabbit
New Features
Bug Fixes