feat: Add Metrics to SharpHound to be collected - BED-7080#202
Open
feat: Add Metrics to SharpHound to be collected - BED-7080#202
Conversation
mykeelium
added
the
blocked by SHC PR
|
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)
WalkthroughA Metrics feature was added: new boolean Metrics properties on Flags and Options, and metric subsystem initialization (registry, sink, router, factory, flush timer) in the main application, with disposal on error and completion. Changes
Sequence DiagramsequenceDiagram
participant Main as Main (Sharphound.cs)
participant Registry as MetricRegistry
participant Sink as FileMetricSink
participant Router as MetricRouter
participant Factory as MetricFactory
participant Timer as MetricsFlushTimer
Main->>Registry: Initialize & register defaults
Registry-->>Main: Ready
Main->>Sink: Create (5s flush interval)
Sink-->>Main: Ready
Main->>Router: Create with Sink & LabelCache
Router-->>Main: Ready
Main->>Factory: Set Metrics.Factory
Factory-->>Main: Ready
Main->>Timer: Create bound to Router flush interval
Timer-->>Main: Ready
Main->>Timer: Dispose on error or completion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/Sharphound.cs (1)
42-43: Consider avoiding static mutable state for resource management.Using a static field for the timer creates implicit coupling and makes the code harder to test. Consider passing the timer through the method chain (e.g., via the context object or as a parameter to
StartCollection), or encapsulating metrics lifecycle in a disposable wrapper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Sharphound.cs` around lines 42 - 43, The static mutable MetricsFlushTimer field (_flushTimer) creates global state; replace it by injecting or passing an instance instead: remove the static field and add a MetricsFlushTimer parameter or include it on an existing context object used by StartCollection (update StartCollection signature to accept the timer or context), or encapsulate timer creation/disposal in a new disposable class (e.g., MetricsLifecycle) and instantiate/preserve that instance per Sharphound run; ensure all callers are updated to pass the timer/context and that shutdown logic disposes/stops the timer (update any Stop/Dispose logic) so there is no static mutable state left.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Sharphound.cs`:
- Around line 190-193: The catch block logs the exception then unconditionally
calls _flushTimer.Dispose(), which throws when flags.Metrics is false and
_flushTimer is null; update the catch block in the SharpHound run routine to
safely dispose the timer (e.g., check _flushTimer != null or use the
null-conditional operator) before calling Dispose so the original exception is
not masked—apply this change where _flushTimer is referenced alongside
logger.LogError in the catch handling for SharpHound.
- Line 254: The disposals currently call _flushTimer.Dispose() which throws when
metrics are disabled (_flushTimer is null) and also can be skipped by early
returns; change the direct call to a null-safe disposal (_flushTimer?.Dispose())
and ensure disposal runs on all exit paths by moving the collection/flush logic
into a try-finally (or a dedicated cleanup method) where the finally block
performs _flushTimer?.Dispose() and any other cleanup so early returns still
release resources; also mirror this fix for the catch block that previously used
_flushTimer.Dispose().
- Around line 169-173: The FileMetricSink instantiation currently hardcodes
"metrics.log" and should be constructed using the user's output options; modify
the FileMetricSink call that uses metricsRegistry.Definitions, metricsWriter,
and fileSinkOptions to build the filePath from the existing options (e.g.
options.OutputDirectory and options.OutputPrefix) so the final path mirrors
other SharpHound outputs (e.g. combine OutputDirectory with OutputPrefix +
"metrics.log" when prefix present); ensure you use the same options object
already available in this scope and preserve existing fileSinkOptions and
metricsWriter parameters when passing the computed path to FileMetricSink.
---
Nitpick comments:
In `@src/Sharphound.cs`:
- Around line 42-43: The static mutable MetricsFlushTimer field (_flushTimer)
creates global state; replace it by injecting or passing an instance instead:
remove the static field and add a MetricsFlushTimer parameter or include it on
an existing context object used by StartCollection (update StartCollection
signature to accept the timer or context), or encapsulate timer
creation/disposal in a new disposable class (e.g., MetricsLifecycle) and
instantiate/preserve that instance per Sharphound run; ensure all callers are
updated to pass the timer/context and that shutdown logic disposes/stops the
timer (update any Stop/Dispose logic) so there is no static mutable state left.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9186961f-337a-4670-b12d-6ee119cd2b81
📒 Files selected for processing (3)
src/Client/Flags.cssrc/Options.cssrc/Sharphound.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Integrate with the metrics logic that is available in SharpHoundCommon.
Motivation and Context
Resolves BED-7080
How Has This Been Tested?
This has been tested by creating a build locally and running collection using the added functionality for metrics.
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit