-
-
Notifications
You must be signed in to change notification settings - Fork 253
Made ReadUserSessionLogs stream logs with IAsyncEnumerable #11540
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe user session logs retrieval is converted from batch-based RPC to SignalR streaming. The server method now uses Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client<br/>(UsersPage)
participant Hub as Server<br/>(AppHub)
participant DB as Database
rect rgb(230, 245, 240)
Note over Client,DB: New Streaming Approach
Client->>Hub: StreamAsync("GetUserSessionLogs", id, token)
activate Hub
Hub->>DB: Query connection ID (with token)
DB-->>Hub: Connection ID
loop For Each Log
Hub->>Hub: Fetch next log via InvokeAsync
Hub->>Hub: Check cancellation
Hub-->>Client: yield DiagnosticLogDto
activate Client
Client->>Client: Enqueue to DiagnosticLogger.Store
deactivate Client
end
Hub-->>Client: Stream complete
deactivate Hub
Client->>Client: Publish SHOW_DIAGNOSTIC_MODAL
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Management/UsersPage.razor.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs(2 hunks)
🔇 Additional comments (4)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/SignalR/AppHub.cs (3)
2-2: LGTM!The
System.Runtime.CompilerServicesnamespace is required for the[EnumeratorCancellation]attribute used in the method signature.
70-73: LGTM!The method signature correctly implements the streaming pattern with
IAsyncEnumerable<DiagnosticLogDto>and proper cancellation support via[EnumeratorCancellation].
75-81: LGTM!The cancellation token is correctly passed to the database query, and
yield breakis the proper way to exit early from an async enumerable method.src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Pages/Management/UsersPage.razor.cs (1)
196-204: LGTM! Client-side streaming implementation is correct.The client properly consumes the streaming API using
await foreachwithStreamAsync<DiagnosticLogDto>. Logs are processed incrementally with proper cancellation support, and the diagnostic modal is displayed after streaming completes.Note: While this implementation is correct, the streaming won't prevent the original SignalR message size error due to the server-side issue where logs are still fetched all at once from the target client (see comments on AppHub.cs).
| var logs = await Clients.Client(userSessionSignalRConnectionId) | ||
| .InvokeAsync<DiagnosticLogDto[]>(SignalRMethods.UPLOAD_DIAGNOSTIC_LOGGER_STORE, cancellationToken); | ||
|
|
||
| if (logs is null || logs.Length is 0) | ||
| yield break; | ||
|
|
||
| foreach (var log in logs) | ||
| { | ||
| cancellationToken.ThrowIfCancellationRequested(); | ||
| yield return log; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation doesn't solve the root cause of the SignalR message size error.
The PR aims to fix the "maximum message size of 32768B was exceeded" error when reading user session logs. However, this implementation only streams logs from the hub to the admin client, but the error occurs when the target user's client returns all logs at once to the hub via InvokeAsync<DiagnosticLogDto[]>.
Architecture flow:
- Admin client →
GetUserSessionLogs→ Hub - Hub →
InvokeAsync<DiagnosticLogDto[]>→ Target client⚠️ 32KB error occurs here - Target client → returns all logs at once → Hub
- Hub → yields logs one by one → Admin client ✅ (streaming works here, but too late)
Impact: Users will still encounter the same SignalR message size error for sessions with large log volumes.
To properly fix this issue, the target client (the one whose logs are being read) must also stream its logs or batch them into smaller chunks. Consider:
- Implementing a streaming method on the client side that yields logs incrementally
- Having the client batch logs into smaller arrays (e.g., 100 logs per batch)
- Implementing a different retrieval mechanism that doesn't rely on a single InvokeAsync call with a potentially large payload
closes #11480
Summary by CodeRabbit