Added Reset for correct time in log#220
Conversation
Added watch.Reset(); for corrected time in log
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR resets the ChangesStopwatch Reset in Search Result Processing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Runtime/CollectionTask.cs (1)
131-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIncomplete stopwatch reset coverage leads to timing accumulation in error paths.
The
watch.Reset()on line 144 is only reached when an item is successfully processed and written. Two code paths skip the reset:
- Null processed objects (line 136-137): When
processed == null, thecontinueskips the reset, leaving the stopwatch stopped but not cleared.- Exceptions (line 146-149): Any exception after
Start()bypasses the reset, potentially leaving the watch running or stopped with accumulated time.Since
Stopwatch.Start()resumes from the previous elapsed time rather than resetting to zero, the next iteration will log cumulative time instead of per-item time.Example failure:
- Item A processes in 100ms but returns null → no reset, stopped at 100ms
- Item B calls
Start()(resumes from 100ms), processes in 50ms,Stop()at 150ms- Log incorrectly shows "took 150ms" instead of 50ms for Item B
⏱️ Recommended fix: reset immediately after logging
Move the reset to line 135 (right after logging) to ensure it executes in all code paths:
log.LogTrace("Consumer {ThreadID} took {time} ms to process {obj}", threadId, watch.Elapsed.TotalMilliseconds, res.DisplayName); +watch.Reset(); if (processed == null) continue; if (processed is Domain d && _context.CollectedDomainSids.Contains(d.ObjectIdentifier)) { d.Properties.Add("collected", true); } await _outputChannel.Writer.WriteAsync(processed); -watch.Reset();Alternative (even cleaner): Replace
Start()+Reset()withRestart()at line 131:log.LogTrace("Consumer {ThreadID} started processing {obj} ({type})", threadId, res.DisplayName, res.ObjectType); -watch.Start(); +watch.Restart(); var processed = await processor.ProcessObject(item, res); watch.Stop(); log.LogTrace("Consumer {ThreadID} took {time} ms to process {obj}", threadId, watch.Elapsed.TotalMilliseconds, res.DisplayName); if (processed == null) continue; if (processed is Domain d && _context.CollectedDomainSids.Contains(d.ObjectIdentifier)) { d.Properties.Add("collected", true); } await _outputChannel.Writer.WriteAsync(processed); -watch.Reset();Using
Restart()(which resets and starts in one call) eliminates the manual reset entirely and handles all edge cases.🤖 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/Runtime/CollectionTask.cs` around lines 131 - 144, The stopwatch handling in CollectionTask (watch started before calling processor.ProcessObject and reset only after writing) can accumulate elapsed time when processed == null or exceptions occur; update the timing so each iteration measures only that item by replacing the Start()+Reset() pattern with watch.Restart() before calling processor.ProcessObject (or, alternatively, move watch.Reset() immediately after the log.LogTrace call) so the stopwatch is always reset regardless of the processed null path or exceptions; ensure references touched include watch, processor.ProcessObject(item, res), log.LogTrace(..., watch.Elapsed.TotalMilliseconds, ...), processed and _outputChannel.Writer.WriteAsync(processed).
🤖 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.
Outside diff comments:
In `@src/Runtime/CollectionTask.cs`:
- Around line 131-144: The stopwatch handling in CollectionTask (watch started
before calling processor.ProcessObject and reset only after writing) can
accumulate elapsed time when processed == null or exceptions occur; update the
timing so each iteration measures only that item by replacing the
Start()+Reset() pattern with watch.Restart() before calling
processor.ProcessObject (or, alternatively, move watch.Reset() immediately after
the log.LogTrace call) so the stopwatch is always reset regardless of the
processed null path or exceptions; ensure references touched include watch,
processor.ProcessObject(item, res), log.LogTrace(...,
watch.Elapsed.TotalMilliseconds, ...), processed and
_outputChannel.Writer.WriteAsync(processed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 385f0ec0-2e81-4a9e-ab1e-af3ce4e779d3
📒 Files selected for processing (1)
src/Runtime/CollectionTask.cs
Reset the watch timer only after processing an item.
fabiodefilipposoftware
left a comment
fabiodefilipposoftware
left a comment
There was a problem hiding this comment.
Corrected position of watch.Reset();
1 participant
Added watch.Reset(); for corrected time in log
Description
Motivation and Context
This PR addresses: [GitHub issue or Jira ticket number]
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit