Add logging for HTTP failures #179
Conversation
Signed-off-by: Dave Thaler <[email protected]>
|
Warning Rate limit exceeded@dthaler has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 25 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe pull request introduces significant changes across several files in the OrcanodeMonitor project. The Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 15
🧹 Outside diff range and nitpick comments (9)
OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs (1)
Line range hint 17-17: Fix logger type parameter.
The logger is typed for IndexModel but should be typed for the current class DataplicityNodeModel.
Apply this change:
-private readonly ILogger<IndexModel> _logger;
+private readonly ILogger<DataplicityNodeModel> _logger;OrcanodeMonitor/Core/MezmoFetcher.cs (5)
124-129: Log a warning when no hosts data is retrieved.
If jsonArray is empty, the method returns silently. Logging a warning can aid in diagnosing issues with data retrieval.
Consider adding a log message:
if (jsonArray.IsNullOrEmpty())
{
+ logger.LogWarning("No hosts data retrieved from Mezmo.");
// Error so do nothing.
return;
}175-178: Implement creation of new nodes when missing.
The TODO suggests creating a node if one is not found for a Mezmo name. Implementing this will help track new nodes automatically.
Would you like assistance in implementing node creation for new Mezmo hosts?
234-239: Log a warning when no views data is retrieved.
Similar to the hosts data, if no views data is retrieved, logging a warning can help identify issues during data fetching.
Consider adding a log message:
if (jsonArray.IsNullOrEmpty())
{
+ logger.LogWarning("No views data retrieved from Mezmo.");
// Error so do nothing.
return;
}292-295: Implement creation of new nodes from Mezmo views.
As noted in the TODO, creating nodes when they are not found can improve the monitoring system's comprehensiveness.
Would you like assistance in implementing node creation for new Mezmo view hosts?
333-337: Consistent use of ILogger interface.
In UpdateMezmoDataAsync, consider using the generic ILogger<MezmoFetcher> for consistency and to take advantage of category-based logging.
Apply this diff to use the generic logger:
- public async static Task UpdateMezmoDataAsync(OrcanodeMonitorContext context, ILogger logger)
+ public async static Task UpdateMezmoDataAsync(OrcanodeMonitorContext context, ILogger<MezmoFetcher> logger)OrcanodeMonitor/Core/Fetcher.cs (3)
332-333: Avoid duplication in error logging for empty serial
The error message for an empty serial is logged twice in lines 328 and 333. This can clutter the logs with redundant information.
Combine the conditions to log the error once:
if (serial.ToString().IsNullOrEmpty())
{
logger.LogError($"Empty serial in UpdateDataplicityDataAsync result");
continue;
}Remove the redundant check and logging.
780-780: Simplify nullable DateTime assignment
The assignment node.LatestRecordedUtc = latestRecorded.HasValue ? latestRecorded.Value.ToUniversalTime() : null; can be simplified using the null-conditional operator.
Simplify the assignment:
-node.LatestRecordedUtc = latestRecorded.HasValue ? latestRecorded.Value.ToUniversalTime() : null;
+node.LatestRecordedUtc = latestRecorded?.ToUniversalTime();256-256: Consider adding more context to exception logs
Logging only the exception in UpdateOrcaHelloDataAsync may not provide enough context. Including additional information can aid in debugging.
Include relevant context in the log message:
-logger.LogError(ex, "Exception in UpdateOrcaHelloDataAsync");
+logger.LogError(ex, $"Exception in UpdateOrcaHelloDataAsync while processing URL: {_orcaHelloHydrophonesUrl}");📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- OrcanodeMonitor/Core/Fetcher.cs (18 hunks)
- OrcanodeMonitor/Core/MezmoFetcher.cs (1 hunks)
- OrcanodeMonitor/Core/PeriodicTasks.cs (1 hunks)
- OrcanodeMonitor/Pages/DataplicityNode.cshtml.cs (1 hunks)
- OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (2 hunks)
🧰 Additional context used
📓 Learnings (2)
OrcanodeMonitor/Core/Fetcher.cs (1)
Learnt from: dthaler
PR: orcasound/orcanode-monitor#162
File: OrcanodeMonitor/Core/Fetcher.cs:579-582
Timestamp: 2024-10-17T20:47:37.157Z
Learning: In `OrcanodeMonitor/Core/Fetcher.cs`, `SaveChangesAsync()` is called after each update (e.g., in methods `UpdateMezmoHostsAsync` and `UpdateMezmoViewsAsync`), ensuring that duplicate events are not logged.
OrcanodeMonitor/Pages/NodeEvents.cshtml.cs (1)
Learnt from: dthaler
PR: orcasound/orcanode-monitor#173
File: OrcanodeMonitor/Pages/NodeEvents.cshtml.cs:0-0
Timestamp: 2024-10-25T18:29:41.800Z
Learning: In the `NodeEventsModel` class, the ID is part of the route, so the functions `OnGet` and `OnPost` cannot be called with an empty ID.
🔇 Additional comments (5)
OrcanodeMonitor/Core/PeriodicTasks.cs (1)
Line range hint 14-61: LGTM! Robust background service implementation.
The class follows best practices with proper error handling, cancellation support, and configurable polling frequency.
OrcanodeMonitor/Core/MezmoFetcher.cs (1)
216-217: Verify necessity of calling SaveChangesAsync.
Before calling SaveChangesAsync, ensure that there are changes pending to save. Unnecessary calls can impact performance.
Please review whether changes have been made to the context that require saving at this point.
OrcanodeMonitor/Core/Fetcher.cs (3)
572-572: Handle potential changes in dataplicity_id appropriately
When dataplicity_id changes, the current implementation logs a warning. However, it might be necessary to update the DataplicitySerial or handle the change explicitly to maintain data integrity.
Review the logic to ensure that changes in dataplicity_id are correctly processed. Consider updating the DataplicitySerial if appropriate.
652-652: Ensure consistent use of SaveChangesAsync()
Per previous learnings, calling SaveChangesAsync() after each update prevents duplicate events from being logged. In UpdateOrcasoundDataAsync, SaveChangesAsync() is called only at the end.
Review whether SaveChangesAsync() should be called after significant updates within loops to persist changes immediately and prevent potential issues with concurrent modifications.
610-615:
Handle potential null references in UpdateOrcasoundSiteDataAsync
If dataArray is null, calling EnumerateArray() could throw an exception.
Add a null check before enumerating:
if (dataArray.HasValue)
{
foreach (JsonElement feed in dataArray.Value.EnumerateArray())
{
UpdateOrcasoundNode(feed, foundList, unfoundList, context, site, logger);
}
}
+else
+{
+ logger.LogError("Data array is null in UpdateOrcasoundSiteDataAsync");
+}Likely invalid or redundant comment.
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Signed-off-by: Dave Thaler <[email protected]>
Summary by CodeRabbit
New Features
MezmoFetcherclass for fetching and processing data from the Mezmo API.Bug Fixes
Documentation
Refactor
Fetcherclass, streamlining the codebase.