[PM-26401] Add logging logic #6523
Conversation
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6523 +/- ##
==========================================
+ Coverage 51.86% 51.99% +0.12%
==========================================
Files 1902 1906 +4
Lines 84058 84273 +215
Branches 7501 7518 +17
==========================================
+ Hits 43598 43818 +220
+ Misses 38766 38751 -15
- Partials 1694 1704 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Claude finished @JimmyVo16's task —— View job PR Review CompleteI've completed a comprehensive review of the changes in this PR. Here are my findings: Summary of ChangesThis PR adds diagnostic logging to two event endpoints (
Critical Issues1. Potential IP Address Logging (Security)Severity: High The Location:
Concern: Recommendation:
2. Inconsistent Method NamingSeverity: Medium Both overloaded methods are named Location:
Recommendation: // Option 1: More descriptive names
LogPublicEventsAggregateData(...) // for public API version
LogInternalEventsAggregateData(...) // for internal version
// Option 2: Indicate the model type
LogAggregateDataForPublicResponse(...)
LogAggregateDataForInternalResponse(...)3. Double SemicolonSeverity: Low Location:
Recommendation: Suggested Improvements4. Potential Performance Issue with OrderingSeverity: Medium Location:
The code orders the entire result set by date just to get the oldest and newest records: var orderedRecords = data.OrderBy(e => e.Date).ToList();
var recordCount = orderedRecords.Count;
var newestRecordDate = orderedRecords.LastOrDefault()?.Date.ToString("o");
var oldestRecordDate = orderedRecords.FirstOrDefault()?.Date.ToString("o");Issue:
Recommendation: var recordCount = data.Count();
DateTime? newestRecordDate = null;
DateTime? oldestRecordDate = null;
if (data.Any())
{
newestRecordDate = data.Max(e => e.Date);
oldestRecordDate = data.Min(e => e.Date);
}
logger.LogInformation(
"Events query for Organization:{OrgId}. Event count:{Count} newest record:{newestRecord} oldest record:{oldestRecord}...",
organizationId,
recordCount,
newestRecordDate?.ToString("o"),
oldestRecordDate?.ToString("o"),
// ... rest
);This approach:
5. Inconsistent Null HandlingSeverity: Low Location:
The code safely handles empty collections with Recommendation: 6. Redundant Count CalculationSeverity: Low Location:
After calling Current: var orderedRecords = data.OrderBy(e => e.Date).ToList();
var recordCount = orderedRecords.Count;Alternative (if keeping the sort): var recordCount = data.Count(); // Get count first (from IEnumerable)
var orderedRecords = data.OrderBy(e => e.Date).ToList();Though if you implement suggestion #4, this becomes moot. 7. Test Coverage GapsSeverity: Low Location:
The tests are comprehensive but could cover a few more edge cases: Missing test scenarios:
Recommendation: [Theory, BitAutoData]
public void LogAggregateData_WithPublicResponse_ExceptionThrown_LogsWarning(...)
{
// Arrange: Mock logger.LogInformation to throw
// Assert: Verify LogWarning was called
}
[Theory, BitAutoData]
public void LogAggregateData_WithPublicResponse_NullDataWithFlagEnabled_HandlesGracefully(...)
{
// Arrange: null data with feature flag enabled
// Assert: No exception, warning is logged
}8. Logging Format ConsistencySeverity: Low Location:
The log messages have inconsistent spacing:
Recommendation: 9. Lack of DocumentationSeverity: Low Location:
The class and methods lack XML documentation comments explaining:
Recommendation: /// <summary>
/// Diagnostic logger for event query operations to help identify discrepancies between endpoints.
/// Logs only aggregate metadata (counts, date ranges, filters) - no PII or event content.
/// Feature-flagged by <see cref="FeatureFlagKeys.EventDiagnosticLogging"/>.
/// </summary>
public static class EventDiagnosticLogger
{
/// <summary>
/// Logs aggregate diagnostic data for public API event queries.
/// </summary>
public static void LogAggregateData(...)Good Practices Observed
Test Coverage AnalysisCurrent Coverage: 68.66% (21 lines missing) The codecov report shows missing coverage in:
The missing lines in controllers are expected (integration-level logging calls), but the 8 missing lines in Security AssessmentOverall: Safe, with minor concerns
Performance AssessmentOverall: Acceptable, with optimization opportunity
Action Items for AuthorMust Fix (before merge):
Should Fix (before or soon after merge): Nice to Have: Adherence to Bitwarden Standards
Overall Assessment: This is a solid implementation of diagnostic logging for the stated purpose. The code is well-tested and follows good practices. The critical issues are minor and easily addressed. The main optimization opportunity is around the sorting operation, which could impact performance with large event collections. |
r-tome
left a comment
r-tome
left a comment
There was a problem hiding this comment.
Looks good to me. My only suggestion would be to change the method names on EventDiagnosticLogger to be clearer on the difference between them.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-26401
📔 Objective
Add logging to both GET /public/events and GET /organizations/{id}/events to capture organizationId, startTime and endTime query parameters, total records retrieved, and other relevant fields to validate whether there is a discrepancy between the two endpoints.
📸 Screenshots
Sanity check
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes