feat: Add Metrics to SharpHound to be collected - BED-7080 #267
Conversation
… LabelValues to a struct
|
Warning Rate limit exceeded@mykeelium has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 48 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. 📒 Files selected for processing (4)
WalkthroughAdds optional latency observation callbacks to timeout/sampling APIs and a new metrics subsystem: definitions, registry, router, sinks, aggregators, writer, file sink, label-value interning, periodic flush timer, and LDAP instrumentation to emit latency/concurrency/failure metrics. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant LDAP as LdapConnectionPool
participant Timeout as AdaptiveTimeout
participant Router as MetricRouter
participant Cache as LabelValuesCache
participant Sink as FileMetricSink
rect rgb(200,220,240)
Note over App,LDAP: Application issues LDAP request
App->>LDAP: SendRequest()
LDAP->>Timeout: ExecuteWithTimeout(func, latencyObservation)
Timeout->>Timeout: execute func, measure elapsed
Timeout->>LDAP: latencyObservation(elapsed)
Timeout-->>LDAP: Result
end
rect rgb(240,230,210)
Note over LDAP,Router: Emit metrics
LDAP->>Router: Observe(definitionId, value, labelValues)
Router->>Cache: Intern(labelValues)
Cache-->>Router: internedValues
Router->>Sink: Observe(DoubleMetricObservation)
Sink->>Sink: Aggregate value
end
rect rgb(220,240,220)
Note over Router,Sink: Periodic flush
Router->>Sink: Flush()
Sink->>Sink: Snapshot & format metrics
Sink-->>_System_: Write output (file/textwriter)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
…ts.cs and DefaultLabelValuesCacheTests.cs
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/CommonLib/AdaptiveTimeout.cs (1)
160-169: AddlatencyObservationparameter toExecuteNetAPIWithTimeoutandExecuteRPCWithTimeoutmethods for API consistency.The
ExecuteWithTimeoutoverloads (lines 67, 90, 114, 137) support optionallatencyObservationcallbacks, butExecuteNetAPIWithTimeout(line 160) and bothExecuteRPCWithTimeoutoverloads (lines 183, 206) do not. This creates an inconsistent API surface where callers can observe latency for general operations but not for NetAPI or RPC operations, despite the underlyingExecutionTimeSampler.SampleExecutionTimesupporting this capability for all variants.Add the optional
Action<double> latencyObservation = nullparameter to these three methods and pass it through to the sampler calls to maintain consistency.src/CommonLib/LdapConnectionPool.cs (1)
1097-1120: Thread-safety concern withLdapMetrics.InFlightRequestsdecrement on exception paths.If
SendRequestWithTimeoutthrows before the decrement on line 1110,InFlightRequestswill remain incremented indefinitely. Consider wrapping the decrement in atry-finallyblock to ensure proper accounting.🔎 Proposed fix to ensure InFlightRequests is always decremented
private async Task<SearchResponse> SendRequestWithTimeout(LdapConnection connection, SearchRequest request, AdaptiveTimeout adaptiveTimeout) { // Prerequest metrics var concurrentRequests = Interlocked.Increment(ref LdapMetrics.InFlightRequests); _metric.Observe(LdapMetricDefinitions.ConcurrentRequests, concurrentRequests, new LabelValues([nameof(LdapConnectionPool), _poolIdentifier])); - - // Add padding to account for network latency and processing overhead - const int TimeoutPaddingSeconds = 3; - var timeout = adaptiveTimeout.GetAdaptiveTimeout(); - var timeoutWithPadding = timeout + TimeSpan.FromSeconds(TimeoutPaddingSeconds); - var result = await adaptiveTimeout.ExecuteWithTimeout((_) => connection.SendRequestAsync(request, timeoutWithPadding), latencyObservation: LatencyObservation); - - // Postrequest metrics - concurrentRequests = Interlocked.Decrement(ref LdapMetrics.InFlightRequests); - _metric.Observe(LdapMetricDefinitions.ConcurrentRequests, concurrentRequests, - new LabelValues([nameof(LdapConnectionPool), _poolIdentifier])); - _metric.Observe(LdapMetricDefinitions.RequestsTotal, 1, - new LabelValues([nameof(LdapConnectionPool), _poolIdentifier])); - - if (result.IsSuccess) - return (SearchResponse)result.Value; - else - throw new TimeoutException($"LDAP {request.Scope} query to '{request.DistinguishedName}' timed out after {timeout.TotalMilliseconds}ms."); + + try { + // Add padding to account for network latency and processing overhead + const int TimeoutPaddingSeconds = 3; + var timeout = adaptiveTimeout.GetAdaptiveTimeout(); + var timeoutWithPadding = timeout + TimeSpan.FromSeconds(TimeoutPaddingSeconds); + var result = await adaptiveTimeout.ExecuteWithTimeout((_) => connection.SendRequestAsync(request, timeoutWithPadding), latencyObservation: LatencyObservation); + + if (result.IsSuccess) + return (SearchResponse)result.Value; + else + throw new TimeoutException($"LDAP {request.Scope} query to '{request.DistinguishedName}' timed out after {timeout.TotalMilliseconds}ms."); + } + finally { + // Postrequest metrics - always decrement in-flight counter + concurrentRequests = Interlocked.Decrement(ref LdapMetrics.InFlightRequests); + _metric.Observe(LdapMetricDefinitions.ConcurrentRequests, concurrentRequests, + new LabelValues([nameof(LdapConnectionPool), _poolIdentifier])); + _metric.Observe(LdapMetricDefinitions.RequestsTotal, 1, + new LabelValues([nameof(LdapConnectionPool), _poolIdentifier])); + } }
🤖 Fix all issues with AI Agents
In @src/CommonLib/Services/DefaultLabelValuesCache.cs:
- Around line 31-33: MakeKey currently joins string[] values with Separator
which allows collisions when values contain the separator; update MakeKey in
DefaultLabelValuesCache (and any inverse Parse/Lookup code) to use a robust
encoding such as escaping instances of Separator in each value (e.g., replace
Separator with an escape sequence) or use a length-prefixing scheme (prefix each
value with its length) before concatenation, and update any code that decodes
the key accordingly so MakeKey and its consumers handle encoded/decoded values
symmetrically.
In @src/CommonLib/Services/FileMetricSink.cs:
- Around line 91-93: The Dispose method in FileMetricSink currently just
disposes _textWriter and can drop any buffered metrics in _states; update
FileMetricSink.Dispose to call Flush() (or otherwise persist/flush the contents
of _states) before disposing _textWriter, ensure Flush is safe to call multiple
times (idempotent) and handle/propagate any exceptions appropriately so metrics
are not lost during dispose.
In @src/CommonLib/Services/MetricAggregator.cs:
- Around line 40-60: The aggregator is not thread-safe: concurrent Observe calls
mutate _bucketCounts, _count and _sum and SnapshotHistogram exposes the internal
_bucketCounts array. Fix by introducing a private readonly sync object (e.g.,
_sync) and wrap the body of Observe in lock(_sync) so _bucketCounts[idx]++,
_count++ and _sum += value are atomic relative to each other; also modify
SnapshotHistogram (and Snapshot()) to lock(_sync) while creating the
HistogramSnapshot and pass a defensive copy of _bucketCounts (e.g., clone the
array) instead of the internal reference to prevent external mutation.
In @src/CommonLib/Services/MetricWriter.cs:
- Around line 33-38: The histogram line construction creates two separate label
blocks when labelText is non-empty; update the logic around builder.AppendFormat
(the block that formats definition.Name + "_bucket" using labelText and
snapshot.Bounds[i]) to merge labels: if labelText is non-empty, insert the le
label before the closing brace (e.g., replace the trailing '}' with
',le="{value}"}') otherwise emit a new {le="..."} block; ensure this change is
applied where cumulativeValue and snapshot.Bounds[i] are used so output becomes
a single combined label block like metric_bucket{method:GET,le="1"} = 5.
In @src/CommonLib/Static/DefaultMetricRegistry.cs:
- Around line 7-39: The RegisterDefaultMetrics extension currently ignores the
boolean return from IMetricRegistry.TryRegister causing silent failures (leaving
LdapMetricDefinitions.* as MetricId.InvalidId); update RegisterDefaultMetrics to
check each TryRegister result and when it returns false either log an error
(including the definition Name and which metric failed) or throw an exception to
fail fast; reference the TryRegister calls that populate
LdapMetricDefinitions.RequestsTotal, FailedRequests, ConcurrentRequests,
RequestLatency, and UnresolvablePrincipals and ensure you surface the failure
for each so callers no longer get silent MetricId.InvalidId values.
In @src/CommonLib/Static/Metrics.cs:
- Around line 15-17: LdapMetrics.InFlightRequests is a plain int and suffers
lost updates under concurrency; change it to a private long backing field and
provide atomic operations (e.g., Interlocked.Increment/Interlocked.Decrement and
Interlocked.Read) via public methods or properties (e.g.,
LdapMetrics.IncrementInFlight(), LdapMetrics.DecrementInFlight(), and
LdapMetrics.GetInFlight()) so all increments/decrements/read use Interlocked to
be thread-safe, or if you must keep a public member, document and enforce use of
Interlocked on the public field.
In @test/unit/MetricAggregatorTests.cs:
- Around line 49-56: The first assertion in the HistogramSnapshot branch
incorrectly compares ehs.TotalCount to itself; update the assertion in the block
that checks "if (expectedSnapshot is HistogramSnapshot ehs && snapshot is
HistogramSnapshot ahs)" so that the Assert.Equal call compares ehs.TotalCount
(expected) to ahs.TotalCount (actual) instead of ehs.TotalCount to
ehs.TotalCount, leaving the other Assert.Equal calls for Sum, Bounds, and Counts
unchanged.
🧹 Nitpick comments (14)
src/CommonLib/ExecutionTimeSampler.cs (1)
46-54: Consider exception handling for latencyObservation callback.If the
latencyObservationcallback throws an exception,AddTimeSamplewill not be invoked, resulting in sample loss for the statistical aggregation. Consider wrapping the callback invocation in a try-catch block to ensure timing samples are recorded regardless of callback behavior, or document that callers must not throw from this callback.🔎 Proposed fix to add defensive exception handling
public async Task<T> SampleExecutionTime<T>(Func<Task<T>> func, Action<double> latencyObservation = null) { var stopwatch = Stopwatch.StartNew(); var result = await func.Invoke(); stopwatch.Stop(); - latencyObservation?.Invoke(stopwatch.ElapsedMilliseconds); + try { + latencyObservation?.Invoke(stopwatch.ElapsedMilliseconds); + } catch { + // Suppress callback exceptions to ensure sample is recorded + } AddTimeSample(stopwatch.Elapsed); return result; }Apply the same pattern to the other three overloads (lines 56-62, 64-72, 74-80).
test/unit/AdaptiveTimeoutTest.cs (1)
30-30: Unnecessary early return statements.The
return;statements at lines 30, 55, 86, 120, and 159 are redundant as they immediately follow assertions with no subsequent code. These can be safely removed to simplify the test methods.🔎 Proposed cleanup
Assert.InRange(observedLatency, 0.0, 60); - return; void LatencyObservation(double latency) {Apply the same removal to lines 55, 86, 120, and 159.
Also applies to: 55-55, 86-86, 120-120, 159-159
src/CommonLib/Models/FileMetricSinkOptions.cs (1)
7-7: Consider validating the timestamp format.The
TimestampFormatstring is not validated and could be set to an invalid format, causing aFormatExceptionat runtime whenDateTimeOffset.ToString()is called during metric flushing.🔎 Proposed validation approach
Add validation in the setter to fail early:
- public string TimestampFormat { get; set; } = "yyyy-MM-dd HH:mm:ss.fff"; + private string _timestampFormat = "yyyy-MM-dd HH:mm:ss.fff"; + public string TimestampFormat { + get => _timestampFormat; + set { + // Validate format by attempting to format a sample timestamp + try { + _ = DateTimeOffset.Now.ToString(value); + _timestampFormat = value; + } catch (FormatException ex) { + throw new ArgumentException($"Invalid timestamp format: {value}", nameof(value), ex); + } + } + }Alternatively, document the expected format and acceptable values in an XML comment.
src/CommonLib/Services/MetricsFlushTimer.cs (2)
22-28: Consider logging swallowed exceptions for observability.Silently catching all exceptions masks failures and makes debugging production issues difficult. At minimum, consider logging to a diagnostics channel or incrementing an error counter.
🔎 Proposed improvement
private void FlushSafe() { try { _flush(); - } catch { - // catch all exception and do not kill the process + } catch (Exception ex) { + // Log the exception but don't kill the timer thread + System.Diagnostics.Debug.WriteLine($"MetricsFlushTimer flush failed: {ex.Message}"); } }
30-32: Dispose doesn't wait for in-flight flush callbacks.If
Dispose()is called while a flush is in progress, the callback may still be executing. For graceful shutdown, consider usingTimer.Dispose(WaitHandle)to wait for completion.src/CommonLib/Models/MetricObservation.cs (1)
3-4: Private constructor on abstract record is redundant.Abstract records cannot be instantiated directly. The private constructor adds no protection beyond what
abstractalready provides.🔎 Simplified version
-public abstract record MetricObservation { - private MetricObservation() { } +public abstract record MetricObservation;test/unit/MetricRouterTests.cs (1)
62-85: Consider adding a test for empty sinks array.The flush test is good, but consider adding coverage for edge cases like an empty sinks collection to ensure
MetricRouterhandles that gracefully.src/CommonLib/Services/MetricFactory.cs (1)
5-9: Consider null validation on constructor parameter.If
routeris null,CreateMetricRouter()will return null, which may cause NullReferenceExceptions downstream.🔎 Proposed improvement
-public sealed class MetricFactory(IMetricRouter router) : IMetricFactory { - private readonly IMetricRouter _router = router; +public sealed class MetricFactory : IMetricFactory { + private readonly IMetricRouter _router; + + public MetricFactory(IMetricRouter router) { + _router = router ?? throw new ArgumentNullException(nameof(router)); + } public IMetricRouter CreateMetricRouter() => _router; }src/CommonLib/Services/MetricWriter.cs (1)
61-69: Output format inconsistency between histogram and default metrics.Histogram metrics output values directly (e.g.,
= 5) whileDefaultAppendwraps values in braces (= {5}). If this is intentional for distinguishing metric types in the output file, consider adding a brief comment. Otherwise, align the formats.test/unit/DefaultLabelValuesCacheTests.cs (1)
36-37: Accessing internal_cachefield in tests.While accessing
internalfields in unit tests is acceptable for verifying internal state, consider whether exposing aCountproperty onDefaultLabelValuesCachewould provide a cleaner public API for testing cache size without exposing implementation details.src/CommonLib/Services/MetricRouter.cs (1)
8-14: Redundant field for_labelCachewith primary constructor.With C# primary constructors,
labelCacheis already captured as a hidden field. The explicit_labelCachefield creates a duplicate. Either remove the explicit field or switch to a traditional constructor if you prefer explicit fields.🔎 Option 1: Remove redundant field
public sealed class MetricRouter( IReadOnlyList<MetricDefinition> definitions, IEnumerable<IMetricSink> sinks, ILabelValuesCache labelCache) : IMetricRouter { private readonly int _definitionCount = definitions.Count; private readonly IMetricSink[] _sinks = sinks.ToArray(); - private readonly ILabelValuesCache _labelCache = labelCache; // ... public void Observe(int definitionId, double value, LabelValues labelValues) { if ((uint)definitionId >= (uint)_definitionCount) return; - var interned = _labelCache.Intern(labelValues.Values); + var interned = labelCache.Intern(labelValues.Values);test/unit/MetricDefinitionTests.cs (1)
87-101: Test structure is slightly confusing - sorting after construction.The test sorts
initBucketsafter constructing the definition to create the expected values. While this works, it might be clearer to use a separate expected array or document why the sort is done post-construction.🔎 Clearer alternative
[Fact] public void CumulativeHistogramDefinition_NormalizesBuckets() { // setup double[] initBuckets = [5, 4, 3, 2, 1]; + double[] expectedBuckets = [1, 2, 3, 4, 5]; // act var definition = new CumulativeHistogramDefinition("name", initBuckets, []); - Array.Sort(initBuckets); // assert - Assert.Equal(initBuckets.Length, definition.Buckets.Length); + Assert.Equal(expectedBuckets.Length, definition.Buckets.Length); for (var i = 0; i < definition.Buckets.Length; ++i) { - Assert.Equal(initBuckets[i], definition.Buckets[i]); + Assert.Equal(expectedBuckets[i], definition.Buckets[i]); } }test/unit/MetricAggregatorTests.cs (1)
60-81: Remove unused helper method.The
snapShotArraysmethod is defined but never called anywhere in the test file. Consider removing it to reduce dead code.🔎 Proposed fix
- private string snapShotArrays(double[] bounds, long[] counts) { - var builder = new StringBuilder(); - builder.Append("bounds: [ "); - Iterate(builder, bounds); - builder.Append(" ], counts: [ "); - Iterate(builder, counts); - builder.Append(" ]"); - return builder.ToString(); - - - void Iterate<T>(StringBuilder sb, T[] os) { - var first = true; - - for (var i = 0; i < os.Length; i++) { - if (!first) - builder.Append(", "); - - builder.Append(os[i]); - first = false; - } - } - }src/CommonLib/Services/MetricAggregator.cs (1)
31-36: Thread-safety concern: GaugeAggregator assignment is not atomic.The
_value = valueassignment is not thread-safe fordoubleon 32-bit systems. While modern 64-bit runtimes handle this atomically, consider usingInterlocked.Exchangefor explicit thread-safety if this will be called from multiple threads.🔎 Proposed fix for thread-safe gauge updates
public sealed class GaugeAggregator : MetricAggregator { private double _value; - public override void Observe(double value) => _value = value; + public override void Observe(double value) => Interlocked.Exchange(ref _value, value); - public override object Snapshot() => _value; + public override object Snapshot() => Volatile.Read(ref _value); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
src/CommonLib/AdaptiveTimeout.cssrc/CommonLib/ExecutionTimeSampler.cssrc/CommonLib/Interfaces/ILabelValuesCache.cssrc/CommonLib/Interfaces/IMetricFactory.cssrc/CommonLib/Interfaces/IMetricRegistry.cssrc/CommonLib/Interfaces/IMetricRouter.cssrc/CommonLib/Interfaces/IMetricSink.cssrc/CommonLib/Interfaces/IMetricWriter.cssrc/CommonLib/LdapConnectionPool.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Models/FileMetricSinkOptions.cssrc/CommonLib/Models/IsExternalInit.cssrc/CommonLib/Models/MetricDefinition.cssrc/CommonLib/Models/MetricObservation.cssrc/CommonLib/Services/DefaultLabelValuesCache.cssrc/CommonLib/Services/FileMetricSink.cssrc/CommonLib/Services/MetricAggregator.cssrc/CommonLib/Services/MetricFactory.cssrc/CommonLib/Services/MetricRegistry.cssrc/CommonLib/Services/MetricRouter.cssrc/CommonLib/Services/MetricWriter.cssrc/CommonLib/Services/MetricsFlushTimer.cssrc/CommonLib/Static/DefaultMetricRegistry.cssrc/CommonLib/Static/Metrics.cstest/unit/AdaptiveTimeoutTest.cstest/unit/CommonLibHelperTests.cstest/unit/DefaultLabelValuesCacheTests.cstest/unit/FileMetricSinkTests.cstest/unit/MetricAggregatorTests.cstest/unit/MetricDefinitionTests.cstest/unit/MetricRegistryTests.cstest/unit/MetricRouterTests.cstest/unit/Utils.cs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-11T20:15:16.238Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/ExecutionTimeSampler.cs:35-43
Timestamp: 2025-07-11T20:15:16.238Z
Learning: In SharpHoundCommon ExecutionTimeSampler statistical methods, the user prefers to keep simple implementations without thread safety snapshots because precise values aren't needed for analysis purposes - they're comfortable with potentially imprecise values from concurrent access.
Applied to files:
src/CommonLib/ExecutionTimeSampler.cs
📚 Learning: 2025-07-15T17:45:25.688Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
Applied to files:
test/unit/AdaptiveTimeoutTest.cssrc/CommonLib/Services/MetricsFlushTimer.cs
📚 Learning: 2025-06-26T16:38:49.677Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 217
File: src/CommonLib/Timeout.cs:17-36
Timestamp: 2025-06-26T16:38:49.677Z
Learning: In SharpHoundCommonLib's Timeout.cs, the team initially had concerns about disposing CancellationTokenSource instances before tasks could check cancellation tokens, but they understand that orphaned tasks (those that exceed timeout) won't be impacted by disposed tokens since their results are already ignored by the timeout handler. They prefer proper resource management with using statements for CancellationTokenSource disposal.
Applied to files:
src/CommonLib/Services/MetricsFlushTimer.cs
📚 Learning: 2025-10-17T13:43:46.833Z
Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.
Applied to files:
src/CommonLib/LdapUtils.cssrc/CommonLib/LdapConnectionPool.cssrc/CommonLib/Static/Metrics.cs
📚 Learning: 2025-06-26T16:59:06.972Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 217
File: src/CommonLib/LdapConnectionPool.cs:677-677
Timestamp: 2025-06-26T16:59:06.972Z
Learning: In SharpHoundCommonLib's LdapConnectionPool.cs, the team acknowledges that method `GetConnectionForSpecificServerAsync` has misleading naming - it contains "Async" in the name but is actually synchronous and blocks on async code, which they recognize as confusing but may need to live with due to backwards compatibility constraints.
Applied to files:
src/CommonLib/LdapConnectionPool.cs
🧬 Code graph analysis (25)
src/CommonLib/ExecutionTimeSampler.cs (1)
src/CommonLib/AdaptiveTimeout.cs (7)
Task(67-76)Task(90-99)Task(114-123)Task(137-146)Task(160-169)Task(183-192)Task(206-215)
src/CommonLib/Interfaces/IMetricSink.cs (2)
src/CommonLib/Services/MetricRouter.cs (4)
Observe(19-30)Observe(42-44)Flush(32-35)Flush(46-48)src/CommonLib/Interfaces/IMetricRouter.cs (2)
Observe(6-6)Flush(7-7)
test/unit/MetricRouterTests.cs (2)
src/CommonLib/Services/MetricRouter.cs (1)
MetricRouter(8-36)src/CommonLib/Models/MetricObservation.cs (1)
MetricObservation(4-4)
src/CommonLib/Services/DefaultLabelValuesCache.cs (1)
src/CommonLib/Interfaces/ILabelValuesCache.cs (1)
Intern(4-4)
test/unit/CommonLibHelperTests.cs (1)
test/unit/Utils.cs (4)
WindowsOnlyTheory(94-98)WindowsOnlyTheory(95-97)WindowsOnlyFact(86-92)WindowsOnlyFact(88-91)
src/CommonLib/Interfaces/IMetricRouter.cs (5)
src/CommonLib/Interfaces/IMetricFactory.cs (1)
IMetricRouter(4-4)src/CommonLib/Services/MetricFactory.cs (2)
IMetricRouter(8-8)IMetricRouter(14-14)src/CommonLib/Services/FileMetricSink.cs (2)
Observe(40-51)Flush(55-89)src/CommonLib/Services/MetricRouter.cs (4)
Observe(19-30)Observe(42-44)Flush(32-35)Flush(46-48)src/CommonLib/Interfaces/IMetricSink.cs (2)
Observe(6-6)Flush(7-7)
src/CommonLib/Interfaces/ILabelValuesCache.cs (1)
src/CommonLib/Services/DefaultLabelValuesCache.cs (1)
Intern(13-29)
test/unit/Utils.cs (1)
test/unit/CommonLibHelperTests.cs (1)
WindowsOnlyTheory(305-323)
test/unit/DefaultLabelValuesCacheTests.cs (1)
src/CommonLib/Services/DefaultLabelValuesCache.cs (3)
DefaultLabelValuesCache(7-36)MakeKey(31-33)Intern(13-29)
src/CommonLib/Services/MetricsFlushTimer.cs (3)
src/CommonLib/LdapConnectionPool.cs (2)
TimeSpan(668-672)Dispose(809-813)src/CommonLib/Services/FileMetricSink.cs (1)
Dispose(91-93)src/CommonLib/LdapUtils.cs (1)
Dispose(1156-1158)
src/CommonLib/Static/DefaultMetricRegistry.cs (3)
src/CommonLib/Services/MetricRegistry.cs (1)
TryRegister(15-26)src/CommonLib/Interfaces/IMetricRegistry.cs (1)
TryRegister(7-7)src/CommonLib/Static/Metrics.cs (1)
LdapMetricDefinitions(24-30)
src/CommonLib/Services/MetricWriter.cs (3)
src/CommonLib/Interfaces/IMetricWriter.cs (1)
StringBuilderAppendMetric(9-16)src/CommonLib/Services/MetricAggregator.cs (7)
MetricAggregator(9-16)MetricAggregator(19-22)CumulativeHistogramAggregator(40-61)Snapshot(21-21)Snapshot(28-28)Snapshot(35-35)Snapshot(57-57)src/CommonLib/Models/MetricDefinition.cs (1)
ToDisplayString(8-28)
test/unit/MetricDefinitionTests.cs (1)
src/CommonLib/Models/MetricDefinition.cs (1)
ToDisplayString(8-28)
test/unit/FileMetricSinkTests.cs (3)
src/CommonLib/Models/MetricObservation.cs (1)
MetricObservation(4-4)src/CommonLib/Models/FileMetricSinkOptions.cs (1)
FileMetricSinkOptions(5-9)src/CommonLib/ConcurrentHashSet.cs (1)
Contains(44-46)
src/CommonLib/LdapUtils.cs (5)
src/CommonLib/Static/Metrics.cs (3)
Metrics(6-13)LdapMetricDefinitions(24-30)LdapMetrics(15-17)src/CommonLib/Processors/PortScanner.cs (3)
PortScanner(9-96)PortScanner(14-16)PortScanner(18-21)src/CommonLib/Services/FileMetricSink.cs (1)
Observe(40-51)src/CommonLib/Services/MetricRouter.cs (2)
Observe(19-30)Observe(42-44)src/CommonLib/Interfaces/IMetricRouter.cs (1)
Observe(6-6)
src/CommonLib/Services/MetricRouter.cs (4)
src/CommonLib/Interfaces/IMetricRouter.cs (2)
Observe(6-6)Flush(7-7)src/CommonLib/Interfaces/IMetricSink.cs (2)
Observe(6-6)Flush(7-7)src/CommonLib/Interfaces/ILabelValuesCache.cs (1)
Intern(4-4)src/CommonLib/Models/MetricObservation.cs (1)
MetricObservation(4-4)
src/CommonLib/Services/MetricFactory.cs (2)
src/CommonLib/Interfaces/IMetricFactory.cs (1)
IMetricRouter(4-4)src/CommonLib/Services/MetricRouter.cs (2)
NoOpMetricRouter(38-49)NoOpMetricRouter(40-40)
test/unit/MetricRegistryTests.cs (3)
test/unit/MetricAggregatorTests.cs (1)
Fact(26-33)src/CommonLib/Services/MetricRegistry.cs (2)
MetricRegistry(8-29)Seal(28-28)src/CommonLib/Static/Metrics.cs (1)
MetricId(20-22)
src/CommonLib/Static/Metrics.cs (1)
src/CommonLib/Services/MetricFactory.cs (2)
NoOpMetricFactory(11-15)NoOpMetricFactory(13-13)
src/CommonLib/Interfaces/IMetricRegistry.cs (1)
src/CommonLib/Services/MetricRegistry.cs (1)
TryRegister(15-26)
src/CommonLib/Interfaces/IMetricWriter.cs (2)
src/CommonLib/Services/MetricWriter.cs (1)
StringBuilderAppendMetric(9-17)src/CommonLib/Services/MetricAggregator.cs (2)
MetricAggregator(9-16)MetricAggregator(19-22)
src/CommonLib/Services/MetricRegistry.cs (3)
src/CommonLib/Interfaces/IMetricRegistry.cs (1)
TryRegister(7-7)src/CommonLib/Static/Metrics.cs (1)
MetricId(20-22)src/CommonLib/ConcurrentHashSet.cs (1)
Add(26-28)
src/CommonLib/Services/FileMetricSink.cs (6)
src/CommonLib/Models/FileMetricSinkOptions.cs (1)
FileMetricSinkOptions(5-9)src/CommonLib/Services/MetricAggregator.cs (6)
MetricAggregator(9-16)MetricAggregator(19-22)Observe(20-20)Observe(27-27)Observe(34-34)Observe(45-55)src/CommonLib/Services/MetricRouter.cs (4)
Observe(19-30)Observe(42-44)Flush(32-35)Flush(46-48)src/CommonLib/Interfaces/IMetricRouter.cs (2)
Observe(6-6)Flush(7-7)src/CommonLib/Models/MetricObservation.cs (1)
MetricObservation(4-4)src/CommonLib/Interfaces/IMetricWriter.cs (1)
StringBuilderAppendMetric(9-16)
src/CommonLib/Services/MetricAggregator.cs (3)
src/CommonLib/Services/FileMetricSink.cs (1)
Observe(40-51)src/CommonLib/Services/MetricRouter.cs (2)
Observe(19-30)Observe(42-44)src/CommonLib/Interfaces/IMetricSink.cs (1)
Observe(6-6)
src/CommonLib/AdaptiveTimeout.cs (2)
src/CommonLib/ExecutionTimeSampler.cs (4)
Task(46-54)Task(56-62)T(64-72)SampleExecutionTime(74-80)src/CommonLib/LdapConnectionPool.cs (11)
Task(74-81)Task(456-486)Task(742-758)Task(771-774)Task(776-793)Task(815-907)Task(909-916)Task(918-952)Task(1000-1064)Task(1072-1086)Task(1097-1120)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (37)
test/unit/CommonLibHelperTests.cs (2)
305-323: LGTM! Appropriate platform gating for Windows-specific APIs.The test uses
SecurityIdentifier, which is only available on Windows. Using[WindowsOnlyTheory]ensures the test is skipped on non-Windows platforms rather than failing at runtime.
325-329: LGTM! Consistent platform gating.The test correctly uses
[WindowsOnlyFact]to skip on non-Windows platforms whereSecurityIdentifieris unavailable.test/unit/Utils.cs (1)
94-98: LGTM! Clean implementation for platform-conditional Theory tests.The
WindowsOnlyTheoryattribute correctly mirrorsWindowsOnlyFactand enables Theory-based tests (with inline data) to be skipped on non-Windows platforms. The implementation is consistent and follows the same pattern as the existingWindowsOnlyFact.src/CommonLib/Models/IsExternalInit.cs (1)
1-11: LGTM! Standard polyfill for C# 9 records on .NET Framework.This implementation correctly follows the established pattern for enabling C# 9 init-only properties and records when targeting .NET Framework. The type is properly scoped as internal, hidden from IntelliSense, and placed in the required namespace with the exact name the compiler expects.
src/CommonLib/Interfaces/IMetricSink.cs (1)
1-8: LGTM! Clean interface design.The interface is well-designed with appropriate use of the
inmodifier for the observation parameter, avoiding unnecessary struct copies while maintaining immutability guarantees.src/CommonLib/Services/MetricRegistry.cs (1)
8-29: Document that metric registration is single-threaded.The
MetricRegistryuses non-thread-safe collections without synchronization. While the code contains potential race conditions in the check-then-act pattern (lines 15-26), the current usage pattern shows metrics are registered only during initialization and sealed before concurrent access occurs.To clarify the contract, add documentation to the class and
TryRegistermethod stating that registration is not thread-safe and must be completed before the registry is sealed or accessed concurrently.Alternatively, if concurrent registration becomes a future requirement, add a
lockstatement around the registration logic (lines 15-26).src/CommonLib/Interfaces/IMetricFactory.cs (1)
3-5: LGTM!Clean factory interface for creating metric routers. The single-method contract is straightforward and appropriate for the factory pattern.
src/CommonLib/Interfaces/ILabelValuesCache.cs (1)
3-5: LGTM!The interface contract is clear. The
Internmethod name follows the established convention for string deduplication/caching patterns.src/CommonLib/Interfaces/IMetricRouter.cs (1)
5-8: LGTM!The interface provides a clean contract for metric observation and flushing. The method signatures are straightforward and appropriate for the routing pattern.
src/CommonLib/Interfaces/IMetricRegistry.cs (1)
6-9: LGTM!The interface follows the Try pattern correctly with the boolean return and out parameter. The read-only collection property provides appropriate access to registered definitions.
src/CommonLib/Models/FileMetricSinkOptions.cs (1)
5-9: Clean options class with sensible defaults.The sealed class with auto-properties and reasonable default values follows standard .NET options pattern conventions.
src/CommonLib/Interfaces/IMetricWriter.cs (1)
8-17: LGTM!The interface is well-defined with a clear single responsibility. The default timestamp format parameter provides sensible defaults while allowing customization.
src/CommonLib/Models/MetricObservation.cs (1)
6-9: LGTM - nested struct design is appropriate for value semantics.The
readonly record structprovides efficient pass-by-value semantics for the hot path. Note thatLabelsValuesarray contents can be mutated externally, but the interning pattern inDefaultLabelValuesCacheprovides immutability guarantees at the usage site.src/CommonLib/Services/DefaultLabelValuesCache.cs (1)
13-29: LGTM - thread-safe interning with proper copy semantics.The implementation correctly copies input arrays before storing, protecting against external mutation. The lock ensures thread safety for concurrent access.
test/unit/MetricRouterTests.cs (2)
11-35: LGTM - good boundary testing for definition IDs.The test correctly validates that IDs outside the valid range (negative, equal to count, greater than count) are rejected without caching or observing.
37-60: LGTM - validates interning and multi-sink observation.The test properly verifies that label values are interned exactly once and that each sink receives the observation.
src/CommonLib/Services/MetricFactory.cs (1)
11-15: LGTM - clean singleton implementation for no-op factory.The NoOpMetricFactory correctly uses the singleton pattern and delegates to
NoOpMetricRouter.Instancefor consistent no-op behavior.src/CommonLib/Static/DefaultMetricRegistry.cs (1)
27-32: Histogram bucket boundaries look reasonable for LDAP latency.The buckets
[100, 250, 500, 1000, 2500, 5000]milliseconds provide good granularity for typical LDAP operation latencies, from fast (sub-100ms) to slow (5+ seconds) queries.test/unit/MetricRegistryTests.cs (1)
1-65: LGTM!The tests provide good coverage for the
MetricRegistrybehavior:
- Validates sequential ID assignment on successful registration
- Confirms name-based deduplication returns the same ID
- Verifies that sealing prevents further registrations and returns
InvalidIdsrc/CommonLib/Static/Metrics.cs (1)
6-13: Good defensive null handling on Factory setter.The null-coalescing fallback to
NoOpMetricFactory.Instanceensures the system gracefully degrades when no factory is configured.test/unit/DefaultLabelValuesCacheTests.cs (1)
52-88: Good concurrency test for validating reference equality.The test effectively validates that the cache returns the same interned reference across concurrent access from multiple threads.
src/CommonLib/Services/MetricRouter.cs (2)
19-30: Efficient bounds check and observation dispatch.The
(uint)definitionId >= (uint)_definitionCounttrick handles both negative IDs and out-of-range IDs in a single branch-friendly comparison. The implementation correctly interns labels before dispatching to sinks.
38-49: LGTM!The
NoOpMetricRoutersingleton pattern is correctly implemented with a private constructor and static instance field.src/CommonLib/Services/FileMetricSink.cs (2)
25-25: Dictionary key relies on reference equality ofstring[]from interning.Using
string[]as part of a dictionary key relies on reference equality, which only works correctly because theMetricRouterinterns label values before passing them to sinks. If any code path bypasses interning, observations with identical labels will create separate aggregators.Consider either:
- Adding a comment documenting this contract
- Using a custom key type with proper
Equals/GetHashCodeimplementationThis works correctly because
MetricRouter.Observecalls_labelCache.Intern(labelValues.Values)before creating the observation. Verify no other code paths can bypass interning.
28-38: File constructor usesFileMode.Create, overwriting existing files.This will truncate any existing file. If append behavior is needed (e.g., for long-running processes that restart), consider making this configurable or using
FileMode.Append.Is overwriting the file on each instantiation the intended behavior?
test/unit/MetricDefinitionTests.cs (1)
1-85: LGTM!Good test coverage for
LabelValuesdisplay formatting edge cases and metric definition construction. The tests validate:
- Empty label names behavior
- Mismatch error formatting for both directions
- Correct label output formatting
- Proper value assignment across all definition types
src/CommonLib/LdapConnectionPool.cs (3)
42-44: LGTM - Metrics field integration.The optional
IMetricRouterinjection with a sensible default viaMetrics.Factory.CreateMetricRouter()follows the existing pattern for_logand maintains backward compatibility.
83-84: LGTM - Clean latency observation callback.The
LatencyObservationhelper method provides a concise way to observe request latency with consistent labeling across all LDAP operations.
127-137: LGTM - Consistent failure metrics tracking.The
FailedRequestsmetric observations are correctly placed in each error/retry path with consistent labeling, providing good visibility into LDAP operation failures.test/unit/MetricAggregatorTests.cs (1)
12-24: LGTM - Good coverage for aggregator factory.The test correctly validates that
MetricAggregatorExtensions.Createreturns the expected aggregator type for each metric definition.src/CommonLib/LdapUtils.cs (2)
53-55: LGTM - Consistent metrics integration pattern.The metrics router injection follows the same pattern as
LdapConnectionPool.cswith proper fallback toMetrics.Factory.CreateMetricRouter()when not provided.Also applies to: 82-96
1143-1145: LGTM - InFlightRequests reset during utility reset.Resetting
LdapMetrics.InFlightRequeststo 0 duringResetUtils()ensures metric state is consistent when the connection pool is reset.test/unit/FileMetricSinkTests.cs (2)
13-19: LGTM - Clean test helper implementation.
SimpleMetricWriterprovides a minimal implementation for testing that captures essential information without complex formatting logic.
54-80: LGTM - Good coverage for autoFlush behavior.The parameterized test correctly validates that
Flush()is only called on the underlying writer whenFlushWriteris true.src/CommonLib/Services/MetricAggregator.cs (1)
24-29: LGTM - Thread-safe counter implementation.
CounterAggregatorcorrectly usesInterlocked.Addfor atomic increments.src/CommonLib/Models/MetricDefinition.cs (2)
7-29: LGTM - Well-designed LabelValues struct.The
ToDisplayStringmethod handles edge cases gracefully, including empty labels and count mismatches with a helpful error message format.
41-54: LGTM - Robust bucket normalization.
NormalizeBucketscorrectly validates input, clones to avoid mutation of the original array, sorts, and validates strictly increasing values. The exception messages are descriptive.
| internal static string MakeKey(string[] values) { | ||
| return values.Length == 1 ? values[0] : string.Join(Separator.ToString(), values); | ||
| } |
There was a problem hiding this comment.
Key collision possible if label values contain the separator character.
If any label value contains \u001F, two distinct label combinations could produce the same key. Consider escaping the separator or using a more robust key generation strategy.
🔎 Example of collision
// These would produce the same key:
MakeKey(["a\u001Fb", "c"]) == MakeKey(["a", "b\u001Fc"])
// Both produce: "a\u001Fb\u001Fc"One fix is to escape the separator:
internal static string MakeKey(string[] values) {
- return values.Length == 1 ? values[0] : string.Join(Separator.ToString(), values);
+ if (values.Length == 1) return values[0];
+ var escaped = values.Select(v => v.Replace(Separator.ToString(), "\\u001F"));
+ return string.Join(Separator.ToString(), escaped);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/CommonLib/Services/DefaultLabelValuesCache.cs around lines 31 - 33,
MakeKey currently joins string[] values with Separator which allows collisions
when values contain the separator; update MakeKey in DefaultLabelValuesCache
(and any inverse Parse/Lookup code) to use a robust encoding such as escaping
instances of Separator in each value (e.g., replace Separator with an escape
sequence) or use a length-prefixing scheme (prefix each value with its length)
before concatenation, and update any code that decodes the key accordingly so
MakeKey and its consumers handle encoded/decoded values symmetrically.
| public static void RegisterDefaultMetrics(this IMetricRegistry registry) { | ||
| // LDAP Metrics | ||
| registry.TryRegister( | ||
| new CounterDefinition( | ||
| Name: "ldap_total_requests", | ||
| LabelNames: ["location", "identifier"]), | ||
| out LdapMetricDefinitions.RequestsTotal); | ||
|
|
||
| registry.TryRegister( | ||
| new CounterDefinition( | ||
| Name: "ldap_failed_requests", | ||
| LabelNames: ["location", "identifier"]), | ||
| out LdapMetricDefinitions.FailedRequests); | ||
|
|
||
| registry.TryRegister( | ||
| new GaugeDefinition( | ||
| Name: "ldap_concurrent_requests", | ||
| LabelNames: ["location", "identifier"]), | ||
| out LdapMetricDefinitions.ConcurrentRequests); | ||
|
|
||
| registry.TryRegister( | ||
| new CumulativeHistogramDefinition( | ||
| Name: "ldap_request_duration_milliseconds", | ||
| InitBuckets: [100, 250, 500, 1000, 2500, 5000], | ||
| LabelNames: ["location", "identifier"]), | ||
| out LdapMetricDefinitions.RequestLatency); | ||
|
|
||
| registry.TryRegister( | ||
| new CounterDefinition( | ||
| Name: "ldap_total_unresolvable_principals", | ||
| LabelNames: ["location"]), | ||
| out LdapMetricDefinitions.UnresolvablePrincipals); | ||
| } |
There was a problem hiding this comment.
Return values from TryRegister are ignored - silent failures possible.
If the registry is sealed or registration fails for any reason, the definition IDs remain MetricId.InvalidId with no indication of failure. Consider logging or throwing when registration fails.
🔎 Proposed improvement with failure handling
public static void RegisterDefaultMetrics(this IMetricRegistry registry) {
// LDAP Metrics
- registry.TryRegister(
+ if (!registry.TryRegister(
new CounterDefinition(
Name: "ldap_total_requests",
LabelNames: ["location", "identifier"]),
- out LdapMetricDefinitions.RequestsTotal);
+ out LdapMetricDefinitions.RequestsTotal)) {
+ // Log warning or throw based on requirements
+ }
// ... similar for other registrations
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/CommonLib/Static/DefaultMetricRegistry.cs around lines 7 - 39, The
RegisterDefaultMetrics extension currently ignores the boolean return from
IMetricRegistry.TryRegister causing silent failures (leaving
LdapMetricDefinitions.* as MetricId.InvalidId); update RegisterDefaultMetrics to
check each TryRegister result and when it returns false either log an error
(including the definition Name and which metric failed) or throw an exception to
fail fast; reference the TryRegister calls that populate
LdapMetricDefinitions.RequestsTotal, FailedRequests, ConcurrentRequests,
RequestLatency, and UnresolvablePrincipals and ensure you surface the failure
for each so callers no longer get silent MetricId.InvalidId values.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @test/unit/AdaptiveTimeoutTest.cs:
- Around line 102-123: The test uses a non-thread-safe List<double> named
observations and a callback LatencyObservation that calls observations.Add
concurrently from multiple ExecuteWithTimeout tasks, causing a race; replace
observations with a thread-safe collection (e.g., ConcurrentBag<double>) or
serialize access (e.g., lock around Add) so concurrent writes from
AdaptiveTimeout.ExecuteWithTimeout tasks are safe, update the LatencyObservation
closure to add to the new thread-safe collection, and keep the rest of the
assertions intact.
- Around line 128-163: The test has a race because completedObservations and
timeoutObservations (written by LatencyCompletedObservation and
LatencyTimeoutObservation callbacks invoked from
AdaptiveTimeout.ExecuteWithTimeout tasks) are plain List<double> and may be
appended from multiple threads; replace them with a thread-safe collection
(e.g., ConcurrentBag<double>) or synchronize writes (lock) and update the two
callback delegates to add into the thread-safe container so concurrent Task
completions cannot corrupt the lists.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/unit/AdaptiveTimeoutTest.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-15T17:45:25.688Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/Processors/LocalGroupProcessor.cs:19-27
Timestamp: 2025-07-15T17:45:25.688Z
Learning: In SharpHoundCommon, the team prefers to keep code simple rather than implement perfect resource management when the resources being managed are non-critical. Specifically, they accept not implementing IDisposable for AdaptiveTimeout instances when the Dispose method is primarily for flushing analytics logs from ExecutionTimeSampler, viewing it as a courtesy rather than a safety requirement.
Applied to files:
test/unit/AdaptiveTimeoutTest.cs
🧬 Code graph analysis (1)
test/unit/AdaptiveTimeoutTest.cs (2)
src/CommonLib/AdaptiveTimeout.cs (10)
TimeSpan(230-244)AdaptiveTimeout(12-351)AdaptiveTimeout(28-47)Task(67-76)Task(90-99)Task(114-123)Task(137-146)Task(160-169)Task(183-192)Task(206-215)src/CommonLib/ExecutionTimeSampler.cs (2)
Task(46-54)Task(56-62)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
test/unit/AdaptiveTimeoutTest.cs (3)
18-36: LGTM - latency observation pattern correctly validates callback invocation.The test properly validates that latency is observed and falls within expected bounds for a 50ms delay. The sentinel value initialization and range assertion pattern is appropriate for verifying callback execution.
38-68: LGTM - sequential execution with individual latency tracking.The test correctly validates latency observation across three sequential timeout executions with adaptive timeout disabled.
70-98: LGTM - validates adaptive timeout calculation with observed latencies.Sequential execution correctly captures individual latencies with appropriate range margins for timing variability.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @src/CommonLib/Models/MetricDefinition.cs:
- Around line 7-32: LabelValues.ToDisplayString currently emits labels as
name:value (e.g., {method:GET}) which breaks OpenMetrics/Prometheus; change the
formatting to name="value" and ensure values are escaped and quoted per the
spec. Update the loop in the ToDisplayString method to append labelNames[i] +
"=" + "\"" + EscapeLabelValue(Values[i]) + "\"" (and similarly for
additionalName/additionalValue), and add a small helper inside the type (e.g.,
private static string EscapeLabelValue(string v)) that escapes backslashes,
double quotes, and newlines (at minimum) before enclosing the value in quotes.
Ensure the improper-count check and empty labelNames behavior remain unchanged
and reference the existing Labels Values array, labelNames parameter, and
additionalName/additionalValue parameters when applying the fix.
In @src/CommonLib/Services/FileMetricSink.cs:
- Around line 91-94: Dispose() currently only flushes and disposes _textWriter,
which loses metrics still buffered in _states; call this class's Flush() method
(which drains/serializes _states to the writer) before calling
_textWriter.Flush() and _textWriter.Dispose() so all observed metrics are
persisted; update the Dispose() implementation in FileMetricSink to invoke
Flush() then flush and dispose _textWriter.
🧹 Nitpick comments (3)
src/CommonLib/LdapUtils.cs (1)
137-146: Good metric placement; consider caching LabelValues.The metric observations correctly track unresolvable principals in error paths, providing valuable observability.
Consider caching the
LabelValuesinstance to avoid repeated allocations since the label is constant across all observations:🔎 Proposed optimization
Add a static readonly field near the top of the class:
+ private static readonly LabelValues LdapUtilsLabel = new LabelValues([nameof(LdapUtils)]); + private readonly ConcurrentDictionary<string, string> _hostResolutionMap = new(StringComparer.OrdinalIgnoreCase);Then update the observation calls (lines 137, 146, and 978):
- _metric.Observe(LdapMetricDefinitions.UnresolvablePrincipals, 1, new LabelValues([nameof(LdapUtils)])); + _metric.Observe(LdapMetricDefinitions.UnresolvablePrincipals, 1, LdapUtilsLabel);src/CommonLib/Services/MetricWriter.cs (1)
12-12: Remove unused variable computation.The
labelTextvariable is computed here but only used in theelsebranch at line 16. In theifbranch (lines 13-14),CumulativeHistogramAppendrecomputes the label text. Consider removing this line and computinglabelTextonly in theelsebranch.🔎 Proposed refactor
public void StringBuilderAppendMetric(StringBuilder builder, MetricDefinition definition, LabelValues labelValues, MetricAggregator aggregator, DateTimeOffset timestamp, string timestampOutputString = "yyyy-MM-dd HH:mm:ss.fff") { - var labelText = labelValues.ToDisplayString(definition.LabelNames); if (aggregator is CumulativeHistogramAggregator cha) { CumulativeHistogramAppend(builder, definition, labelValues, cha, timestamp, timestampOutputString); } else { + var labelText = labelValues.ToDisplayString(definition.LabelNames); DefaultAppend(builder, definition, labelValues.ToDisplayString(definition.LabelNames), aggregator, timestamp, timestampOutputString); } }src/CommonLib/Services/MetricAggregator.cs (1)
31-36: GaugeAggregator lacks thread-safety, inconsistent with other aggregators.
GaugeAggregator.Observe(line 34) directly assigns_valuewithout synchronization, whileCounterAggregatorusesInterlocked(line 27) andCumulativeHistogramAggregatoruses a lock (lines 53-57). This creates an inconsistency in thread-safety guarantees.If aggregators are meant to be independently thread-safe, consider using
Interlocked.Exchangeorvolatilefor the gauge. If aggregators are always protected by external locks (as inFileMetricSink), the synchronization in Counter and Histogram may be unnecessary overhead.Based on learnings, simple implementations without thread safety may be acceptable for statistical values where precise readings aren't critical.
🔎 Option 1: Make Gauge thread-safe
public sealed class GaugeAggregator : MetricAggregator { private double _value; - public override void Observe(double value) => _value = value; - public override object Snapshot() => _value; + public override void Observe(double value) => Interlocked.Exchange(ref _value, value); + public override object Snapshot() => Interlocked.CompareExchange(ref _value, 0, 0); }🔎 Option 2: Document that aggregators rely on external synchronization
Add a comment to the
MetricAggregatorbase class documenting the expected threading model and remove internal synchronization from Counter and Histogram if external locking is guaranteed.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/CommonLib/LdapConnectionPool.cssrc/CommonLib/LdapUtils.cssrc/CommonLib/Models/MetricDefinition.cssrc/CommonLib/Services/FileMetricSink.cssrc/CommonLib/Services/MetricAggregator.cssrc/CommonLib/Services/MetricWriter.cssrc/CommonLib/Static/Metrics.cstest/unit/MetricAggregatorTests.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- test/unit/MetricAggregatorTests.cs
- src/CommonLib/LdapConnectionPool.cs
- src/CommonLib/Static/Metrics.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-17T13:43:46.833Z
Learnt from: MikeX777
Repo: SpecterOps/SharpHoundCommon PR: 241
File: src/CommonLib/Processors/LdapPropertyProcessor.cs:168-169
Timestamp: 2025-10-17T13:43:46.833Z
Learning: Properties added to dictionaries returned by methods in SharpHoundCommon (such as those in LdapPropertyProcessor) may be consumed by dependent projects like SharpHound (SH) and SharpHoundEnterprise (SHE), even if they are not used within the SharpHoundCommon repository itself.
Applied to files:
src/CommonLib/LdapUtils.cs
📚 Learning: 2025-07-11T20:15:16.238Z
Learnt from: definitelynotagoblin
Repo: SpecterOps/SharpHoundCommon PR: 222
File: src/CommonLib/ExecutionTimeSampler.cs:35-43
Timestamp: 2025-07-11T20:15:16.238Z
Learning: In SharpHoundCommon ExecutionTimeSampler statistical methods, the user prefers to keep simple implementations without thread safety snapshots because precise values aren't needed for analysis purposes - they're comfortable with potentially imprecise values from concurrent access.
Applied to files:
src/CommonLib/Services/MetricAggregator.cs
🧬 Code graph analysis (3)
src/CommonLib/LdapUtils.cs (4)
src/CommonLib/Static/Metrics.cs (4)
Metrics(7-14)LdapMetricDefinitions(31-37)LdapMetrics(16-24)ResetInFlight(23-23)src/CommonLib/Services/FileMetricSink.cs (1)
Observe(40-51)src/CommonLib/Interfaces/IMetricRouter.cs (1)
Observe(6-6)src/CommonLib/Services/MetricRouter.cs (2)
Observe(19-30)Observe(42-44)
src/CommonLib/Services/MetricAggregator.cs (4)
src/CommonLib/Services/FileMetricSink.cs (1)
Observe(40-51)src/CommonLib/Services/MetricRouter.cs (2)
Observe(19-30)Observe(42-44)src/CommonLib/Interfaces/IMetricSink.cs (1)
Observe(6-6)src/CommonLib/ConcurrentHashSet.cs (1)
Add(26-28)
src/CommonLib/Services/FileMetricSink.cs (5)
src/CommonLib/Models/FileMetricSinkOptions.cs (1)
FileMetricSinkOptions(5-9)src/CommonLib/Services/MetricAggregator.cs (7)
MetricAggregator(9-16)MetricAggregator(19-22)Observe(20-20)Observe(27-27)Observe(34-34)Observe(46-58)MetricAggregatorExtensions(8-17)src/CommonLib/Models/MetricObservation.cs (1)
MetricObservation(4-4)src/CommonLib/Services/MetricWriter.cs (1)
StringBuilderAppendMetric(10-18)src/CommonLib/Interfaces/IMetricWriter.cs (1)
StringBuilderAppendMetric(9-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (9)
src/CommonLib/LdapUtils.cs (4)
18-23: LGTM: Using statements added for metrics integration.The new using directives appropriately support the metrics functionality (IMetricRouter interface, LabelValues model, and Metrics static class).
978-978: LGTM: Consistent metric observation in exception path.The metric observation correctly tracks unresolvable distinguished names, maintaining consistency with the other error paths in this file.
1143-1145: LGTM: Metrics reset integrated into utility reset.Resetting the in-flight request counter alongside other cached state is appropriate and maintains consistency across the reset operation.
53-96: LGTM: Metrics field and constructor integration implemented correctly.The metric router field is properly initialized via constructor injection with a factory fallback. The dependency injection pattern is clean and follows existing conventions. The new optional parameter is backward compatible—all existing instantiations in the codebase use the parameterless constructor and are unaffected.
src/CommonLib/Services/MetricWriter.cs (1)
87-88: Verify the intended output format for non-histogram metrics.The format string uses
{{{3}}}which produces output likemetric_name{labels} = {42}with braces around the value. This differs from typical metric formats which usually emit justmetric_name{labels} valueormetric_name{labels} = valuewithout braces around the value. Please confirm this is the intended format.src/CommonLib/Services/FileMetricSink.cs (2)
28-38: LGTM! File path constructor correctly chains to primary constructor.The secondary constructor properly creates a StreamWriter with appropriate file mode and sharing settings, then delegates to the primary constructor.
55-89: LGTM! Flush implementation minimizes lock hold time.The flush logic correctly locks only during metric formatting, then writes the output outside the lock. This is good design for minimizing contention.
src/CommonLib/Services/MetricAggregator.cs (1)
24-29: Verify double-to-long truncation in CounterAggregator.Line 27 casts the
double valueparameter tolong, which truncates any fractional part. If counters are only expected to be incremented by whole numbers, consider changing the signature to acceptlonginstead ofdouble. If fractional increments are valid, the current implementation silently loses precision.src/CommonLib/Models/MetricDefinition.cs (1)
42-58: LGTM! Histogram bucket normalization is correct.The
NormalizeBucketsmethod properly validates non-null/non-empty input, sorts the buckets, and ensures strictly increasing values. The error messages are clear and appropriate.
| public readonly record struct LabelValues(string[] Values) { | ||
| public string ToDisplayString(IReadOnlyList<string> labelNames, string additionalName = null, string additionalValue = null) { | ||
| if (labelNames.Count == 0) | ||
| return string.Empty; | ||
|
|
||
| if (labelNames.Count != Values.Length) | ||
| return $"{{Improper Observation Labels, LabelNamesCount: {labelNames.Count}, LabelValuesCount: {Values.Length}}}"; | ||
|
|
||
| var sb = new StringBuilder(); | ||
| sb.Append('{'); | ||
| for (var i = 0; i < labelNames.Count; i++) { | ||
| if (i > 0) | ||
| sb.Append(','); | ||
|
|
||
| sb.Append(labelNames[i]) | ||
| .Append(':') | ||
| .Append(Values[i]); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(additionalName) && !string.IsNullOrEmpty(additionalValue)) { | ||
| sb.Append(',').Append(additionalName).Append(':').Append(additionalValue); | ||
| } | ||
|
|
||
| sb.Append('}'); | ||
| return sb.ToString(); | ||
| } |
There was a problem hiding this comment.
Label format violates OpenMetrics/Prometheus specification.
Lines 22-23 and 27 produce label output like {method:GET,path:/api}, but the OpenMetrics specification requires labels in the format {method="GET",path="/api"} with:
=instead of:between label names and values- Quoted values (escaped UTF-8 strings)
This format incompatibility means the output cannot be parsed by standard Prometheus/OpenMetrics tools.
🔎 Proposed fix to conform to OpenMetrics spec
public readonly record struct LabelValues(string[] Values) {
public string ToDisplayString(IReadOnlyList<string> labelNames, string additionalName = null, string additionalValue = null) {
if (labelNames.Count == 0)
return string.Empty;
if (labelNames.Count != Values.Length)
return $"{{Improper Observation Labels, LabelNamesCount: {labelNames.Count}, LabelValuesCount: {Values.Length}}}";
var sb = new StringBuilder();
sb.Append('{');
for (var i = 0; i < labelNames.Count; i++) {
if (i > 0)
sb.Append(',');
- sb.Append(labelNames[i])
- .Append(':')
- .Append(Values[i]);
+ sb.Append(labelNames[i])
+ .Append('=')
+ .Append('"')
+ .Append(Values[i].Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("\n", "\\n"))
+ .Append('"');
}
if (!string.IsNullOrEmpty(additionalName) && !string.IsNullOrEmpty(additionalValue)) {
- sb.Append(',').Append(additionalName).Append(':').Append(additionalValue);
+ sb.Append(',').Append(additionalName).Append('=').Append('"')
+ .Append(additionalValue.Replace("\\", "\\\\").Replace("\"", "\\\"").Replace("\n", "\\n"))
+ .Append('"');
}
sb.Append('}');
return sb.ToString();
}
};Note: The escaping shown handles backslash, quote, and newline. The full OpenMetrics spec defines additional escape sequences that may need to be handled.
🤖 Prompt for AI Agents
In @src/CommonLib/Models/MetricDefinition.cs around lines 7 - 32,
LabelValues.ToDisplayString currently emits labels as name:value (e.g.,
{method:GET}) which breaks OpenMetrics/Prometheus; change the formatting to
name="value" and ensure values are escaped and quoted per the spec. Update the
loop in the ToDisplayString method to append labelNames[i] + "=" + "\"" +
EscapeLabelValue(Values[i]) + "\"" (and similarly for
additionalName/additionalValue), and add a small helper inside the type (e.g.,
private static string EscapeLabelValue(string v)) that escapes backslashes,
double quotes, and newlines (at minimum) before enclosing the value in quotes.
Ensure the improper-count check and empty labelNames behavior remain unchanged
and reference the existing Labels Values array, labelNames parameter, and
additionalName/additionalValue parameters when applying the fix.
Description
Created a proof of concept for the ability to collect metrics while collecting as well as extending out the logic to be able to flush metrics to multiple types of sinks.
Motivation and Context
BED-7080
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.