Exception Handling and Examples for Expected Failures #264
Conversation
davidvonthenen
requested review from
lukeocodes,
jpvajda,
SandraRodgers and
CopperBeardy
WalkthroughThe updates across the solution primarily focus on enhancing logging, exception handling, and token management for various components, including REST clients, WebSocket connections, and the microphone streaming example. New projects have been introduced, reflecting a broader test coverage for expected failures and new functionalities. Adjustments in project paths and references aim to streamline the solution structure. The addition of a Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (13)
- Deepgram.Dev.sln (6 hunks)
- Deepgram/Abstractions/AbstractRestClient.cs (17 hunks)
- Deepgram/Clients/Live/v1/Client.cs (12 hunks)
- Deepgram/Clients/Live/v1/Constants.cs (1 hunks)
- Deepgram/Logger/Constants.cs (1 hunks)
- Deepgram/Logger/Log.cs (2 hunks)
- examples/streaming/microphone/Program.cs (6 hunks)
- tests/expected_failures/rest/bad_host/BadHost.csproj (1 hunks)
- tests/expected_failures/rest/bad_host/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/Program.cs (1 hunks)
Additional Context Used
Learnings (1)
Deepgram.Dev.sln (1)
User: dvonthenen" PR: deepgram/deepgram-dotnet-sdk#253 File: Deepgram.Dev.sln:0-0 Timestamp: 2024-03-28T19:45:21.188Z Learning: References to "Prerecorded" and other project names in the documentation and build scripts have been updated as part of the PR changes. Remaining mentions of "Prerecorded" in `Deepgram/README.md`, `examples/README.md`, and `README.md` were noted for potential review.
Additional comments not posted (19)
tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj (1)
11-11: Ensure the updated project reference path"..\..\..\..\Deepgram\Deepgram.csproj"correctly reflects the new directory structure of the project.tests/expected_failures/rest/bad_host/BadHost.csproj (1)
11-11: Ensure the updated project reference path"..\..\..\..\Deepgram\Deepgram.csproj"correctly reflects the new directory structure of the project.tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj (1)
11-11: Ensure the updated project reference path"..\..\..\..\Deepgram\Deepgram.csproj"correctly reflects the new directory structure of the project.Deepgram/Clients/Live/v1/Constants.cs (1)
15-16: The addition ofDefaultConnectTimeoutwith a value of 5000 milliseconds is a sensible enhancement for managing default connect/disconnect timeouts.Deepgram/Logger/Constants.cs (1)
48-51: The addition of theDisableenum value toLogLevelprovides users with the option to explicitly disable logging, enhancing flexibility and control.tests/expected_failures/rest/bad_host/Program.cs (1)
23-23: Consider adding a comment explaining the use of a non-routable IP address (127.0.0.1) inDeepgramHttpClientOptionsto simulate a bad host scenario for demonstration purposes.tests/expected_failures/rest/trigger_cancel/Program.cs (1)
25-25: Consider adding a comment explaining the use of a short delay (250ms) inCancellationTokenSourceto simulate an intentional request cancellation for demonstration purposes.tests/expected_failures/websocket/exercise_timeout/Program.cs (1)
17-17: Consider adding a comment explaining the choice ofLogLevel.Debugfor more verbose logging in demonstration scenarios.examples/streaming/microphone/Program.cs (3)
21-23: Clarify the example for logging initialization to prevent confusion.
Consider adding a comment explaining that the commented line is an alternative way to initialize logging with a different verbosity level.
37-37: The addition of a newline before logging event types enhances the readability of the output.
95-95: Stopping the microphone and cleaning up resources is a good practice for proper resource management.Deepgram.Dev.sln (1)
128-141: Ensure the newly added projects ("trigger_cancel," "bad_host," "websocket," and "rest") and the reorganized "ExerciseTimeout" project are correctly referenced in the documentation and build scripts, reflecting the changes made in this PR.Deepgram/Abstractions/AbstractRestClient.cs (6)
48-54: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [51-86]
Consider using structured logging for better log management and analysis.
- Log.Verbose("AbstractRestClient.GetAsync<T>", "ENTER"); - Log.Debug("GetAsync<T>", $"uriSegment: {uriSegment}"); - Log.Debug("GetAsync<T>", $"addons: {addons}"); + Log.Verbose($"{{Method}} ENTER", "AbstractRestClient.GetAsync<T>"); + Log.Debug($"{{Method}} uriSegment: {{uriSegment}}, addons: {{addons}}", "GetAsync<T>", uriSegment, addons);This approach allows for easier parsing and querying of logs, especially when dealing with complex systems or when troubleshooting.
289-295: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [292-330]
In the
PostAsync<S, T>method, validate thecancellationTokenbefore proceeding with the operation.+ if (cancellationToken == null || cancellationToken.Token.CanBeCanceled == false) + { + throw new ArgumentException("A valid CancellationToken must be provided."); + }This ensures that a valid cancellation token is always passed to the method, enhancing the robustness of cancellation handling.
327-356: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [353-401]
For the
PostAsync<R, S, T>method, consider adding error handling for the case wherecontentis expected to be aStreambut isnull.if (typeof(R) == typeof(Stream)) { Stream? stream = content as Stream; + if (stream == null) + { + throw new ArgumentNullException(nameof(content), "Content cannot be null when expected to be a Stream."); + } request.Content = HttpRequestUtil.CreateStreamPayload(stream); }This addition prevents potential null reference exceptions and ensures that the method's contract is correctly enforced.
427-433: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [430-475]
In the
PatchAsync<S, T>method, consider adding a check to ensure that thecancellationTokenis not already canceled before starting the operation.+ if (cancellationToken != null && cancellationToken.Token.IsCancellationRequested) + { + Log.Information("PatchAsync<S, T>", "Cancellation was requested before the operation started."); + return default(T); + }This preemptive check can save resources by avoiding unnecessary operations when cancellation is already requested.
544-565: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [505-562]
In the
PutAsync<S, T>method, ensure sensitive headers are not logged to avoid potential security risks.if (headers != null) { foreach (var header in headers) { var tmp = header.Key.ToLower(); - if ( !(tmp.Contains("password") || tmp.Contains("token") || tmp.Contains("authorization") || tmp.Contains("auth")) ) + if (tmp.Contains("password") || tmp.Contains("token") || tmp.Contains("authorization") || tmp.Contains("auth")) + { + Log.Debug("PutAsync<S, T>", $"Skipping sensitive header {header.Key}"); + continue; + } request.Headers.Add(header.Key, header.Value); } }This change ensures that sensitive information is not inadvertently exposed in the logs.
605-626: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [574-623]
For the
DeleteAsync<T>method, consider adding a check for the cancellation token's validity to ensure it can be canceled.+ if (cancellationToken == null || cancellationToken.Token.CanBeCanceled == false) + { + throw new ArgumentException("A valid CancellationToken must be provided."); + }This ensures that the method behaves predictably in scenarios where cancellation might be necessary.
Deepgram/Clients/Live/v1/Client.cs (1)
72-87: > 📝 NOTEThis review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [62-83]
In the
Connectmethod, consider using a more descriptive variable name forcancelTokento clarify its purpose.- CancellationTokenSource? cancelToken = null + CancellationTokenSource? connectionCancellationToken = nullRenaming the variable enhances code readability by making the cancellation token's role more explicit.
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (13)
- Deepgram.Dev.sln (6 hunks)
- Deepgram/Abstractions/AbstractRestClient.cs (17 hunks)
- Deepgram/Clients/Live/v1/Client.cs (12 hunks)
- Deepgram/Clients/Live/v1/Constants.cs (1 hunks)
- Deepgram/Logger/Constants.cs (1 hunks)
- Deepgram/Logger/Log.cs (2 hunks)
- examples/streaming/microphone/Program.cs (6 hunks)
- tests/expected_failures/rest/bad_host/BadHost.csproj (1 hunks)
- tests/expected_failures/rest/bad_host/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/Program.cs (1 hunks)
Files skipped from review as they are similar to previous changes (13)
- Deepgram.Dev.sln
- Deepgram/Abstractions/AbstractRestClient.cs
- Deepgram/Clients/Live/v1/Client.cs
- Deepgram/Clients/Live/v1/Constants.cs
- Deepgram/Logger/Constants.cs
- Deepgram/Logger/Log.cs
- examples/streaming/microphone/Program.cs
- tests/expected_failures/rest/bad_host/BadHost.csproj
- tests/expected_failures/rest/bad_host/Program.cs
- tests/expected_failures/rest/trigger_cancel/Program.cs
- tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj
- tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj
- tests/expected_failures/websocket/exercise_timeout/Program.cs
There was a problem hiding this comment.
Actionable comments posted: 0
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (13)
- Deepgram.Dev.sln (6 hunks)
- Deepgram/Abstractions/AbstractRestClient.cs (17 hunks)
- Deepgram/Clients/Live/v1/Client.cs (12 hunks)
- Deepgram/Clients/Live/v1/Constants.cs (1 hunks)
- Deepgram/Logger/Constants.cs (1 hunks)
- Deepgram/Logger/Log.cs (2 hunks)
- examples/streaming/microphone/Program.cs (6 hunks)
- tests/expected_failures/rest/bad_host/BadHost.csproj (1 hunks)
- tests/expected_failures/rest/bad_host/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/Program.cs (1 hunks)
- tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj (1 hunks)
- tests/expected_failures/websocket/exercise_timeout/Program.cs (1 hunks)
Files skipped from review as they are similar to previous changes (13)
- Deepgram.Dev.sln
- Deepgram/Abstractions/AbstractRestClient.cs
- Deepgram/Clients/Live/v1/Client.cs
- Deepgram/Clients/Live/v1/Constants.cs
- Deepgram/Logger/Constants.cs
- Deepgram/Logger/Log.cs
- examples/streaming/microphone/Program.cs
- tests/expected_failures/rest/bad_host/BadHost.csproj
- tests/expected_failures/rest/bad_host/Program.cs
- tests/expected_failures/rest/trigger_cancel/Program.cs
- tests/expected_failures/rest/trigger_cancel/TriggerCancel.csproj
- tests/expected_failures/websocket/exercise_timeout/ExerciseTimeout.csproj
- tests/expected_failures/websocket/exercise_timeout/Program.cs
Addresses issue: #252
This was the stress test to throw as many exceptions and to test edge cases as best as I could. In these scenarios, I found a few issues.
Changes include:
AbstractRestClientjust better logging of the exceptions based on log levelsLiveClient, this is where I found a couple of issues went it came to cancelling. These changes include:Connect()andStop()Each get their own CancelToken to be used for only that operation. That is the token that is passed into the function. That token is optional and a default will be used to timeout those operations in 5 seconds.AbstractRestClient.Summary by CodeRabbit