feat: Add Metrics to SharpHound to be collected - BED-7080#267
feat: Add Metrics to SharpHound to be collected - BED-7080#267
Conversation
… LabelValues to a struct
WalkthroughThis pull request introduces a comprehensive metrics and observability system to the codebase. Changes include new metric interfaces, service implementations for routing, aggregation, and persistence, optional latency observation callbacks for timeout/sampling operations, and integration of metrics into LDAP operations. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MetricRouter
participant LabelCache
participant MetricSink
participant TextWriter
Client->>MetricRouter: Observe(definitionId, value, labelValues)
MetricRouter->>LabelCache: Intern(labelValues.Values)
LabelCache-->>MetricRouter: internedArray
MetricRouter->>MetricSink: Observe(DoubleMetricObservation)
MetricSink->>MetricSink: Create/Update MetricAggregator
MetricSink->>MetricSink: aggregator.Observe(value)
Client->>MetricRouter: Flush()
MetricRouter->>MetricSink: Flush()
MetricSink->>MetricSink: aggregator.Snapshot()
MetricSink->>MetricSink: metricWriter.StringBuilderAppendMetric()
MetricSink->>TextWriter: Write(formatted metric line)
MetricSink->>TextWriter: Flush()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/CommonLib/Models/MetricDefinition.cs (1)
22-30:⚠️ Potential issue | 🟠 MajorEscape label values before writing metric text format.
Line 24 and Line 29 append raw values. Quotes, backslashes, and newlines can produce invalid metric lines.
Proposed fix
sb.Append(labelNames[i]) .Append("=\"") - .Append(Values[i]) + .Append(EscapeLabelValue(Values[i])) .Append('"'); @@ if (!string.IsNullOrEmpty(additionalName) && !string.IsNullOrEmpty(additionalValue)) { - sb.Append(',').Append(additionalName).Append("=\"").Append(additionalValue).Append('"'); + sb.Append(',').Append(additionalName).Append("=\"").Append(EscapeLabelValue(additionalValue)).Append('"'); } @@ } + + private static string EscapeLabelValue(string value) => + value.Replace("\\", "\\\\") + .Replace("\"", "\\\"") + .Replace("\n", "\\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Models/MetricDefinition.cs` around lines 22 - 30, The code appends raw label values (Values[i] and additionalValue) into the metric text output, which can produce invalid metric lines when values contain quotes, backslashes, newlines or returns; add a helper to escape label values (e.g., replace backslash with \\ , double quote with \\" , newline with \\n , carriage return with \\r) and call it where values are appended in MetricDefinition (escape Values[i] when appending with labelNames[i], and escape additionalValue when appending with additionalName); implement the helper as a private static string EscapeLabelValue(string) and use it in the code path that builds the sb.Append(...) fragments.
🧹 Nitpick comments (2)
test/unit/AdaptiveTimeoutTest.cs (1)
32-32: Redundantreturn;statements in test methodsThese
return;lines are unnecessary and add noise; removing them makes the tests easier to scan.Also applies to: 57-57, 88-88, 122-122, 161-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/AdaptiveTimeoutTest.cs` at line 32, Remove the redundant trailing "return;" statements from the test methods in AdaptiveTimeoutTest.cs (they are no-ops that add noise); locate each "return;" in the AdaptiveTimeoutTest class (occurring at the ends of individual test methods) and delete those lines (e.g., the occurrences reported around lines 32, 57, 88, 122, 161) while leaving the method bodies and assertions intact so behavior does not change.test/unit/MetricDefinitionTests.cs (1)
48-84: Add regression tests for escaping and non-finite bucket validation.Current coverage misses two high-risk paths: escaped label values in
ToDisplayStringand rejection ofNaN/Infinitybuckets inCumulativeHistogramDefinition.Also applies to: 112-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/MetricDefinitionTests.cs` around lines 48 - 84, Add regression tests covering two missing paths: ensure LabelValues.ToDisplayString properly escapes special characters by adding a test where label values contain quotes, backslashes, or commas and asserting the output uses correct escaping; and ensure CumulativeHistogramDefinition rejects non-finite bucket boundaries by adding tests that construct a CumulativeHistogramDefinition (or call its validation method) with Double.NaN, Double.PositiveInfinity, and Double.NegativeInfinity buckets and assert that the constructor/validator throws the expected exception (e.g., ArgumentException/ArgumentOutOfRangeException). Reference LabelValues.ToDisplayString for the escaping tests and CumulativeHistogramDefinition (its constructor or Validate/ValidateBuckets method) for the non-finite bucket tests.
🤖 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/CommonLib/Models/MetricDefinition.cs`:
- Around line 66-76: In NormalizeBuckets, after cloning and sorting the array
(variable copy) validate that every element is a finite number: iterate over
copy and throw an ArgumentException if any element is NaN or Infinite (use
double.IsNaN/ double.IsInfinity or double.IsFinite if available) so buckets
cannot contain non-finite values that would break Array.BinarySearch and label
generation; keep the existing strictly-increasing check in the same loop (or a
subsequent check) and include a clear message like "Histogram buckets must be
finite numbers" when throwing.
In `@src/CommonLib/Services/FileMetricSink.cs`:
- Around line 40-46: In Observe(in MetricObservation.DoubleMetricObservation
observation) guard access to _definitions by validating observation.DefinitionId
before indexing: check that _definitions contains the key/index (or that the
index is in range) and if missing, log or ignore the observation and return
instead of indexing; then only call MetricAggregatorExtensions.Create(...) and
modify _states when the definition exists. This prevents a throw when Observe is
called outside MetricRouter validation and keeps behavior localized in the
FileMetricSink class.
- Around line 73-78: The metric lines are not using the configured timestamp
format because StringBuilderAppendMetric is called without passing
_options.TimestampFormat; update the call in FileMetricSink (the invocation of
_metricWriter.StringBuilderAppendMetric(sb, definition, new
LabelValues(labelValues), aggregator, timestamp)) to pass the configured format
(e.g., add _options.TimestampFormat as an argument) or modify
StringBuilderAppendMetric to accept and apply a timestampFormat parameter so
that the same _options.TimestampFormat used for the flush header is applied to
each metric line.
- Around line 55-89: The Flush() method currently builds the output under lock
but calls _textWriter.Write(...) and _textWriter.Flush() outside the lock, which
allows concurrent Flush()/Dispose() to interleave writes; move the calls to
_textWriter.Write(output) and conditional _textWriter.Flush() inside the
existing lock (_lock) so the entire build-and-write sequence is atomic, or
alternatively ensure Dispose() also acquires the same _lock before touching
_textWriter; update FileMetricSink.Flush() and any Dispose() that writes to
_textWriter to use the same lock protection.
In `@src/CommonLib/Services/MetricAggregator.cs`:
- Around line 42-43: The constructor of CumulativeHistogramAggregator currently
stores a reference to the mutable double[] bounds; defensively copy it into a
private readonly double[] (e.g., _bounds = (double[])bounds.Clone()) and use
_bounds when sizing _bucketCounts and computing buckets; likewise, when
producing HistogramSnapshot (the method that returns snapshot/bounds), return a
copy of the bounds (not the internal _bounds reference) so external callers
cannot mutate internal bucket semantics. Ensure any other places that previously
referenced the constructor parameter now use the private _bounds field.
In `@src/CommonLib/Services/MetricWriter.cs`:
- Around line 54-59: The histogram metric formatting in MetricWriter.cs is not
using culture-invariant formatting; update all builder.AppendFormat calls and
timestamp.ToString calls in the MetricWriter class/method that writes histogram
metrics so they use CultureInfo.InvariantCulture (either by calling
ToString(CultureInfo.InvariantCulture) for timestamp, snapshot.Bounds[i],
snapshot.Sum, or by using the AppendFormat overload that accepts an
IFormatProvider). Specifically change the AppendFormat invocations that format
timestamp, definition.Name + "_bucket", snapshot.Bounds[i], cumulativeValue, and
the similar lines that emit _count and _sum to use CultureInfo.InvariantCulture
so numbers and timestamps serialize with invariant culture consistently (mirror
the existing correct use of
snapshot.Bounds[i].ToString(CultureInfo.InvariantCulture)).
- Around line 92-110: The two DefaultAppend overloads
(MetricWriter.DefaultAppend for MetricAggregator<double> and
MetricAggregator<long>) currently emit numeric values wrapped in literal braces
and use culture-sensitive formatting; update both to remove the extra braces
around the value placeholder so the format string renders the raw numeric value,
and format both the timestamp and the aggregator snapshot using
CultureInfo.InvariantCulture (e.g., call
timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture) and
convert the aggregator.Snapshot() to a string with InvariantCulture) so output
is locale-independent and produces " = value" instead of " = {value}".
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Around line 119-121: The range checks over 'observations' are vacuous when the
collection is empty; update the test in AdaptiveTimeoutTest to first assert the
expected count (e.g., Assert.True/Assert.Equal on observations.Count or
observations.Length) before iterating, then perform the existing Assert.InRange
checks; apply the same fix to the second occurrence referenced (the block around
lines 155-160). Ensure you reference the 'observations' collection in both test
blocks so missing callbacks fail the count assertion before value checks.
---
Duplicate comments:
In `@src/CommonLib/Models/MetricDefinition.cs`:
- Around line 22-30: The code appends raw label values (Values[i] and
additionalValue) into the metric text output, which can produce invalid metric
lines when values contain quotes, backslashes, newlines or returns; add a helper
to escape label values (e.g., replace backslash with \\ , double quote with \\"
, newline with \\n , carriage return with \\r) and call it where values are
appended in MetricDefinition (escape Values[i] when appending with
labelNames[i], and escape additionalValue when appending with additionalName);
implement the helper as a private static string EscapeLabelValue(string) and use
it in the code path that builds the sb.Append(...) fragments.
---
Nitpick comments:
In `@test/unit/AdaptiveTimeoutTest.cs`:
- Line 32: Remove the redundant trailing "return;" statements from the test
methods in AdaptiveTimeoutTest.cs (they are no-ops that add noise); locate each
"return;" in the AdaptiveTimeoutTest class (occurring at the ends of individual
test methods) and delete those lines (e.g., the occurrences reported around
lines 32, 57, 88, 122, 161) while leaving the method bodies and assertions
intact so behavior does not change.
In `@test/unit/MetricDefinitionTests.cs`:
- Around line 48-84: Add regression tests covering two missing paths: ensure
LabelValues.ToDisplayString properly escapes special characters by adding a test
where label values contain quotes, backslashes, or commas and asserting the
output uses correct escaping; and ensure CumulativeHistogramDefinition rejects
non-finite bucket boundaries by adding tests that construct a
CumulativeHistogramDefinition (or call its validation method) with Double.NaN,
Double.PositiveInfinity, and Double.NegativeInfinity buckets and assert that the
constructor/validator throws the expected exception (e.g.,
ArgumentException/ArgumentOutOfRangeException). Reference
LabelValues.ToDisplayString for the escaping tests and
CumulativeHistogramDefinition (its constructor or Validate/ValidateBuckets
method) for the non-finite bucket tests.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/CommonLib/Models/MetricDefinition.cssrc/CommonLib/Services/FileMetricSink.cssrc/CommonLib/Services/MetricAggregator.cssrc/CommonLib/Services/MetricWriter.cstest/unit/AdaptiveTimeoutTest.cstest/unit/FileMetricSinkTests.cstest/unit/MetricAggregatorTests.cstest/unit/MetricDefinitionTests.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/MetricAggregatorTests.cs
| private static double[] NormalizeBuckets(double[] buckets) { | ||
| if (buckets is null || buckets.Length == 0) | ||
| throw new ArgumentException("Histogram buckets cannot be empty"); | ||
|
|
||
| var copy = (double[])buckets.Clone(); | ||
| Array.Sort(copy); | ||
|
|
||
| for (var i = 1; i < copy.Length; i++) { | ||
| if (copy[i] <= copy[i - 1]) | ||
| throw new ArgumentException("Histogram buckets must be strictly increasing"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and read the MetricDefinition.cs file
find . -name "MetricDefinition.cs" -type fRepository: SpecterOps/SharpHoundCommon
Length of output: 113
🏁 Script executed:
# Search for NormalizeBuckets method
rg "NormalizeBuckets" -A 5 -B 2Repository: SpecterOps/SharpHoundCommon
Length of output: 979
🏁 Script executed:
# Search for histogram bucket related tests
rg "histogram.*bucket|bucket.*histogram" -i -lRepository: SpecterOps/SharpHoundCommon
Length of output: 189
🏁 Script executed:
# Read the complete MetricDefinition.cs file
cat -n src/CommonLib/Models/MetricDefinition.csRepository: SpecterOps/SharpHoundCommon
Length of output: 3963
🏁 Script executed:
# Read the test file to understand existing tests
cat -n test/unit/MetricDefinitionTests.csRepository: SpecterOps/SharpHoundCommon
Length of output: 5876
🏁 Script executed:
# Search for any handling of infinity or NaN in the codebase
rg "IsNaN|IsInfinity" -B 3 -A 3Repository: SpecterOps/SharpHoundCommon
Length of output: 53
🏁 Script executed:
# Search for MetricAggregator usage to understand how buckets are used
cat -n src/CommonLib/Services/MetricAggregator.cs | head -100Repository: SpecterOps/SharpHoundCommon
Length of output: 3108
🏁 Script executed:
# Search for any histogram generation or "le" label usage
rg "le" -i -B 2 -A 2Repository: SpecterOps/SharpHoundCommon
Length of output: 50385
🏁 Script executed:
# Let's search more specifically for histogram output or prometheus formatting
rg "histogram|prometheus" -i -lRepository: SpecterOps/SharpHoundCommon
Length of output: 342
🏁 Script executed:
# Look for how buckets are actually used in output/serialization
rg "Buckets" -B 3 -A 3 src/CommonLibRepository: SpecterOps/SharpHoundCommon
Length of output: 2493
🏁 Script executed:
# Check if there's any documentation about the histogram format or expected behavior
find . -name "*.md" -o -name "*.txt" | xargs grep -l "histogram\|bucket" 2>/dev/null | head -5Repository: SpecterOps/SharpHoundCommon
Length of output: 53
🏁 Script executed:
# Let's check if NaN/Infinity would cause issues with Array.Sort and Array.BinarySearch
python3 << 'EOF'
# Simulate what happens with NaN/Infinity in sorted arrays
import math
# Test 1: Array.Sort behavior with NaN
buckets = [1.0, 2.0, math.nan, 3.0]
buckets_sorted = sorted(buckets)
print("Original buckets:", [1.0, 2.0, math.nan, 3.0])
print("After sort:", buckets_sorted)
print()
# Test 2: Strictly increasing check
test_arrays = [
[1.0, 2.0, math.inf],
[1.0, math.nan, 2.0],
[-math.inf, 0.0, 1.0],
[1.0, math.nan, math.nan],
]
for arr in test_arrays:
print(f"Testing array: {arr}")
sorted_arr = sorted(arr)
print(f"After sort: {sorted_arr}")
# Check strictly increasing
is_increasing = all(sorted_arr[i] <= sorted_arr[i-1] for i in range(1, len(sorted_arr)))
print(f"Passes strictly increasing check: {not is_increasing}")
# Check what comparison would fail
for i in range(1, len(sorted_arr)):
cmp = sorted_arr[i] <= sorted_arr[i-1]
print(f" sorted_arr[{i}] ({sorted_arr[i]}) <= sorted_arr[{i-1}] ({sorted_arr[i-1]}): {cmp}")
print()
# Test 3: BinarySearch behavior
arr = [1.0, 2.0, math.inf]
search_values = [0.5, 1.5, 2.5, math.nan, math.inf]
print("BinarySearch simulation with array:", arr)
for val in search_values:
# Python's bisect doesn't work well with NaN, but Array.BinarySearch in .NET would have issues
print(f" Searching for {val}: would have unpredictable behavior with NaN")
EOFRepository: SpecterOps/SharpHoundCommon
Length of output: 1330
🏁 Script executed:
cat -n src/CommonLib/Services/MetricWriter.cs | head -150Repository: SpecterOps/SharpHoundCommon
Length of output: 5624
🏁 Script executed:
# Search for how buckets are written to output
rg "Bounds|buckets" -B 2 -A 2 src/CommonLib/Services/MetricWriter.csRepository: SpecterOps/SharpHoundCommon
Length of output: 782
🏁 Script executed:
# Check the test file for histogram tests
cat -n test/unit/MetricAggregatorTests.csRepository: SpecterOps/SharpHoundCommon
Length of output: 5259
Validate histogram bucket values are finite.
Lines 66-76 do not reject NaN or Infinity values. NaN comparisons always evaluate to false, allowing NaN buckets to pass the strictly-increasing check. Non-finite buckets would then corrupt bucket placement in Array.BinarySearch and produce invalid le label values in metric output.
Proposed fix
private static double[] NormalizeBuckets(double[] buckets) {
if (buckets is null || buckets.Length == 0)
throw new ArgumentException("Histogram buckets cannot be empty");
var copy = (double[])buckets.Clone();
Array.Sort(copy);
- for (var i = 1; i < copy.Length; i++) {
- if (copy[i] <= copy[i - 1])
+ for (var i = 0; i < copy.Length; i++) {
+ if (double.IsNaN(copy[i]) || double.IsInfinity(copy[i]))
+ throw new ArgumentException("Histogram buckets must be finite numbers");
+
+ if (i > 0 && copy[i] <= copy[i - 1])
throw new ArgumentException("Histogram buckets must be strictly increasing");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static double[] NormalizeBuckets(double[] buckets) { | |
| if (buckets is null || buckets.Length == 0) | |
| throw new ArgumentException("Histogram buckets cannot be empty"); | |
| var copy = (double[])buckets.Clone(); | |
| Array.Sort(copy); | |
| for (var i = 1; i < copy.Length; i++) { | |
| if (copy[i] <= copy[i - 1]) | |
| throw new ArgumentException("Histogram buckets must be strictly increasing"); | |
| } | |
| private static double[] NormalizeBuckets(double[] buckets) { | |
| if (buckets is null || buckets.Length == 0) | |
| throw new ArgumentException("Histogram buckets cannot be empty"); | |
| var copy = (double[])buckets.Clone(); | |
| Array.Sort(copy); | |
| for (var i = 0; i < copy.Length; i++) { | |
| if (double.IsNaN(copy[i]) || double.IsInfinity(copy[i])) | |
| throw new ArgumentException("Histogram buckets must be finite numbers"); | |
| if (i > 0 && copy[i] <= copy[i - 1]) | |
| throw new ArgumentException("Histogram buckets must be strictly increasing"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Models/MetricDefinition.cs` around lines 66 - 76, In
NormalizeBuckets, after cloning and sorting the array (variable copy) validate
that every element is a finite number: iterate over copy and throw an
ArgumentException if any element is NaN or Infinite (use double.IsNaN/
double.IsInfinity or double.IsFinite if available) so buckets cannot contain
non-finite values that would break Array.BinarySearch and label generation; keep
the existing strictly-increasing check in the same loop (or a subsequent check)
and include a clear message like "Histogram buckets must be finite numbers" when
throwing.
| public void Observe(in MetricObservation.DoubleMetricObservation observation) { | ||
| var key = (observation.DefinitionId, observation.LabelsValues); | ||
|
|
||
| lock (_lock) { | ||
| if (!_states.TryGetValue(key, out var aggregator)) { | ||
| aggregator = MetricAggregatorExtensions.Create(_definitions[observation.DefinitionId]); | ||
| _states[key] = aggregator; |
There was a problem hiding this comment.
Guard against invalid definition IDs at sink boundary.
Line 45 indexes _definitions directly. If this sink is used outside MetricRouter validation, it can throw.
Proposed fix
public void Observe(in MetricObservation.DoubleMetricObservation observation) {
+ if ((uint)observation.DefinitionId >= (uint)_definitions.Length)
+ return;
+
var key = (observation.DefinitionId, observation.LabelsValues);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void Observe(in MetricObservation.DoubleMetricObservation observation) { | |
| var key = (observation.DefinitionId, observation.LabelsValues); | |
| lock (_lock) { | |
| if (!_states.TryGetValue(key, out var aggregator)) { | |
| aggregator = MetricAggregatorExtensions.Create(_definitions[observation.DefinitionId]); | |
| _states[key] = aggregator; | |
| public void Observe(in MetricObservation.DoubleMetricObservation observation) { | |
| if ((uint)observation.DefinitionId >= (uint)_definitions.Length) | |
| return; | |
| var key = (observation.DefinitionId, observation.LabelsValues); | |
| lock (_lock) { | |
| if (!_states.TryGetValue(key, out var aggregator)) { | |
| aggregator = MetricAggregatorExtensions.Create(_definitions[observation.DefinitionId]); | |
| _states[key] = aggregator; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/FileMetricSink.cs` around lines 40 - 46, In Observe(in
MetricObservation.DoubleMetricObservation observation) guard access to
_definitions by validating observation.DefinitionId before indexing: check that
_definitions contains the key/index (or that the index is in range) and if
missing, log or ignore the observation and return instead of indexing; then only
call MetricAggregatorExtensions.Create(...) and modify _states when the
definition exists. This prevents a throw when Observe is called outside
MetricRouter validation and keeps behavior localized in the FileMetricSink
class.
| public void Flush() { | ||
| string output; | ||
| lock (_lock) { | ||
| var sb = new StringBuilder(EstimateSize()); | ||
|
|
||
| var timestamp = DateTimeOffset.Now; | ||
| sb.Append("Metric Flush: ") | ||
| .Append(timestamp.ToString(_options.TimestampFormat)) | ||
| .AppendLine(); | ||
| sb.Append('=', 40).AppendLine(); | ||
|
|
||
| // Must use this deconstruction for .Net Version | ||
| foreach (var kvp in _states) { | ||
| var definitionId = kvp.Key.Item1; | ||
| var labelValues = kvp.Key.Item2; | ||
| var aggregator = kvp.Value; | ||
| var definition = _definitions[definitionId]; | ||
|
|
||
| _metricWriter.StringBuilderAppendMetric( | ||
| sb, | ||
| definition, | ||
| new LabelValues(labelValues), | ||
| aggregator, | ||
| timestamp); | ||
| } | ||
|
|
||
| sb.Append('=', 40).AppendLine().AppendLine().AppendLine().AppendLine().AppendLine(); | ||
| output = sb.ToString(); | ||
| } | ||
|
|
||
| _textWriter.Write(output); | ||
|
|
||
| if (_options.FlushWriter) | ||
| _textWriter.Flush(); | ||
| } |
There was a problem hiding this comment.
Prevent interleaved writes during concurrent flushes.
Line 85-88 run outside synchronization. Concurrent Flush()/Dispose() can interleave TextWriter writes.
Proposed fix
public sealed class FileMetricSink(
@@
private readonly object _lock = new();
+ private readonly object _writeLock = new();
@@
public void Flush() {
string output;
lock (_lock) {
@@
output = sb.ToString();
}
-
- _textWriter.Write(output);
-
- if (_options.FlushWriter)
- _textWriter.Flush();
+
+ lock (_writeLock) {
+ _textWriter.Write(output);
+ if (_options.FlushWriter)
+ _textWriter.Flush();
+ }
}
@@
public void Dispose() {
Flush();
- _textWriter.Dispose();
+ lock (_writeLock) {
+ _textWriter.Dispose();
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/FileMetricSink.cs` around lines 55 - 89, The Flush()
method currently builds the output under lock but calls _textWriter.Write(...)
and _textWriter.Flush() outside the lock, which allows concurrent
Flush()/Dispose() to interleave writes; move the calls to
_textWriter.Write(output) and conditional _textWriter.Flush() inside the
existing lock (_lock) so the entire build-and-write sequence is atomic, or
alternatively ensure Dispose() also acquires the same _lock before touching
_textWriter; update FileMetricSink.Flush() and any Dispose() that writes to
_textWriter to use the same lock protection.
| _metricWriter.StringBuilderAppendMetric( | ||
| sb, | ||
| definition, | ||
| new LabelValues(labelValues), | ||
| aggregator, | ||
| timestamp); |
There was a problem hiding this comment.
Apply configured timestamp format to metric lines too.
Line 73-78 call StringBuilderAppendMetric without _options.TimestampFormat, so only the flush header honors the option.
Proposed fix
_metricWriter.StringBuilderAppendMetric(
sb,
definition,
new LabelValues(labelValues),
aggregator,
- timestamp);
+ timestamp,
+ _options.TimestampFormat);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _metricWriter.StringBuilderAppendMetric( | |
| sb, | |
| definition, | |
| new LabelValues(labelValues), | |
| aggregator, | |
| timestamp); | |
| _metricWriter.StringBuilderAppendMetric( | |
| sb, | |
| definition, | |
| new LabelValues(labelValues), | |
| aggregator, | |
| timestamp, | |
| _options.TimestampFormat); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/FileMetricSink.cs` around lines 73 - 78, The metric
lines are not using the configured timestamp format because
StringBuilderAppendMetric is called without passing _options.TimestampFormat;
update the call in FileMetricSink (the invocation of
_metricWriter.StringBuilderAppendMetric(sb, definition, new
LabelValues(labelValues), aggregator, timestamp)) to pass the configured format
(e.g., add _options.TimestampFormat as an argument) or modify
StringBuilderAppendMetric to accept and apply a timestampFormat parameter so
that the same _options.TimestampFormat used for the flush header is applied to
each metric line.
| public sealed class CumulativeHistogramAggregator(double[] bounds) : MetricAggregator<HistogramSnapshot> { | ||
| private readonly long[] _bucketCounts = new long[bounds.Length + 1]; // Includes the Inf+ bucket |
There was a problem hiding this comment.
Defensively copy histogram bounds to prevent external mutation.
Line 42 captures a mutable double[] reference, and Line 66 returns the same bounds reference in snapshots. External mutation can change bucket semantics after initialization.
Proposed fix
public sealed class CumulativeHistogramAggregator(double[] bounds) : MetricAggregator<HistogramSnapshot> {
+ private readonly double[] _bounds = (double[])bounds.Clone();
private readonly long[] _bucketCounts = new long[bounds.Length + 1]; // Includes the Inf+ bucket
@@
- var idx = Array.BinarySearch(bounds, value);
+ var idx = Array.BinarySearch(_bounds, value);
@@
public HistogramSnapshot SnapshotHistogram() {
lock (_lock) {
- return new HistogramSnapshot(bounds, (long[])_bucketCounts.Clone(), _count, _sum);
+ return new HistogramSnapshot((double[])_bounds.Clone(), (long[])_bucketCounts.Clone(), _count, _sum);
}
}
}Also applies to: 64-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/MetricAggregator.cs` around lines 42 - 43, The
constructor of CumulativeHistogramAggregator currently stores a reference to the
mutable double[] bounds; defensively copy it into a private readonly double[]
(e.g., _bounds = (double[])bounds.Clone()) and use _bounds when sizing
_bucketCounts and computing buckets; likewise, when producing HistogramSnapshot
(the method that returns snapshot/bounds), return a copy of the bounds (not the
internal _bounds reference) so external callers cannot mutate internal bucket
semantics. Ensure any other places that previously referenced the constructor
parameter now use the private _bounds field.
| builder.AppendFormat("{0} {1}{2}{{le=\"{3}\"}} = {4}\n", | ||
| timestamp.ToString(timestampOutputString), | ||
| definition.Name + "_bucket", | ||
| defaultLabelText, | ||
| snapshot.Bounds[i], | ||
| cumulativeValue); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd "MetricWriter.cs"Repository: SpecterOps/SharpHoundCommon
Length of output: 151
🏁 Script executed:
wc -l src/CommonLib/Services/MetricWriter.csRepository: SpecterOps/SharpHoundCommon
Length of output: 113
🏁 Script executed:
cat -n src/CommonLib/Services/MetricWriter.csRepository: SpecterOps/SharpHoundCommon
Length of output: 5624
Apply invariant culture formatting to histogram metrics and timestamps.
Lines 54-59 and 78-82 lack culture-invariant formatting. The AppendFormat() calls, timestamp.ToString() calls, and the snapshot.Bounds[i] and snapshot.Sum properties will serialize with commas on non-en-US locales, breaking metric parsing.
Also fix similar issues at lines 64-65, 71-72, and 84-85 where AppendFormat() and timestamp.ToString() lack CultureInfo.InvariantCulture. Note that line 51 already correctly uses snapshot.Bounds[i].ToString(CultureInfo.InvariantCulture), indicating the pattern should be applied consistently throughout the method.
Proposed fix
- builder.AppendFormat("{0} {1}{2}{{le=\"{3}\"}} = {4}\n",
- timestamp.ToString(timestampOutputString),
+ builder.AppendFormat(CultureInfo.InvariantCulture, "{0} {1}{2}{{le=\"{3}\"}} = {4}\n",
+ timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture),
definition.Name + "_bucket",
defaultLabelText,
- snapshot.Bounds[i],
+ snapshot.Bounds[i].ToString(CultureInfo.InvariantCulture),
cumulativeValue);
...
- builder.AppendFormat("{0} {1}{2} = {3}\n",
- timestamp.ToString(timestampOutputString),
+ builder.AppendFormat(CultureInfo.InvariantCulture, "{0} {1}{2} = {3}\n",
+ timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture),
definition.Name + "_sum",
defaultLabelText,
snapshot.Sum);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| builder.AppendFormat("{0} {1}{2}{{le=\"{3}\"}} = {4}\n", | |
| timestamp.ToString(timestampOutputString), | |
| definition.Name + "_bucket", | |
| defaultLabelText, | |
| snapshot.Bounds[i], | |
| cumulativeValue); | |
| builder.AppendFormat(CultureInfo.InvariantCulture, "{0} {1}{2}{{le=\"{3}\"}} = {4}\n", | |
| timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture), | |
| definition.Name + "_bucket", | |
| defaultLabelText, | |
| snapshot.Bounds[i].ToString(CultureInfo.InvariantCulture), | |
| cumulativeValue); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/MetricWriter.cs` around lines 54 - 59, The histogram
metric formatting in MetricWriter.cs is not using culture-invariant formatting;
update all builder.AppendFormat calls and timestamp.ToString calls in the
MetricWriter class/method that writes histogram metrics so they use
CultureInfo.InvariantCulture (either by calling
ToString(CultureInfo.InvariantCulture) for timestamp, snapshot.Bounds[i],
snapshot.Sum, or by using the AppendFormat overload that accepts an
IFormatProvider). Specifically change the AppendFormat invocations that format
timestamp, definition.Name + "_bucket", snapshot.Bounds[i], cumulativeValue, and
the similar lines that emit _count and _sum to use CultureInfo.InvariantCulture
so numbers and timestamps serialize with invariant culture consistently (mirror
the existing correct use of
snapshot.Bounds[i].ToString(CultureInfo.InvariantCulture)).
| private static void DefaultAppend( | ||
| StringBuilder builder, | ||
| MetricDefinition definition, | ||
| string labelText, | ||
| MetricAggregator<double> aggregator, | ||
| DateTimeOffset timestamp, | ||
| string timestampOutputString) => | ||
| builder.AppendFormat("{0} {1}{2} = {{{3}}}\n", timestamp.ToString(timestampOutputString), | ||
| definition.Name, labelText, aggregator.Snapshot()); | ||
|
|
||
| private static void DefaultAppend( | ||
| StringBuilder builder, | ||
| MetricDefinition definition, | ||
| string labelText, | ||
| MetricAggregator<long> aggregator, | ||
| DateTimeOffset timestamp, | ||
| string timestampOutputString) => | ||
| builder.AppendFormat("{0} {1}{2} = {{{3}}}\n", timestamp.ToString(timestampOutputString), | ||
| definition.Name, labelText, aggregator.Snapshot()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the MetricWriter.cs file
find . -name "MetricWriter.cs" -type fRepository: SpecterOps/SharpHoundCommon
Length of output: 111
🏁 Script executed:
# Read lines 92-110 from the file (approximate range to get full context)
if [ -f "src/CommonLib/Services/MetricWriter.cs" ]; then
cat -n "src/CommonLib/Services/MetricWriter.cs" | sed -n '85,115p'
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 1293
🏁 Script executed:
# Also check imports to see what's available (CultureInfo usage)
if [ -f "src/CommonLib/Services/MetricWriter.cs" ]; then
head -30 "src/CommonLib/Services/MetricWriter.cs"
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 1445
🏁 Script executed:
# Check entire DefaultAppend method definitions to understand context
if [ -f "src/CommonLib/Services/MetricWriter.cs" ]; then
rg -n "DefaultAppend" "src/CommonLib/Services/MetricWriter.cs" -A 10
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 1823
🏁 Script executed:
# Look for CumulativeHistogramAppend to see if it has similar issues
if [ -f "src/CommonLib/Services/MetricWriter.cs" ]; then
rg -n "CumulativeHistogramAppend|AppendFormat" "src/CommonLib/Services/MetricWriter.cs" -B 2 -A 5
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 3478
🏁 Script executed:
# Get full file size to see the complete context
if [ -f "src/CommonLib/Services/MetricWriter.cs" ]; then
wc -l "src/CommonLib/Services/MetricWriter.cs"
fiRepository: SpecterOps/SharpHoundCommon
Length of output: 113
Fix gauge/counter sample rendering to emit raw numeric values and use invariant culture.
Lines 99 and 109 currently produce malformed output due to {{{3}}} in the format string, which wraps numeric values in literal braces (e.g., = {value} instead of = value). Additionally, both timestamp.ToString() and aggregator.Snapshot() are formatted without CultureInfo.InvariantCulture, causing locale-sensitive output (decimal commas in some regions will break the metric format).
Proposed fix
private static void DefaultAppend(
StringBuilder builder,
MetricDefinition definition,
string labelText,
MetricAggregator<double> aggregator,
DateTimeOffset timestamp,
string timestampOutputString) =>
- builder.AppendFormat("{0} {1}{2} = {{{3}}}\n", timestamp.ToString(timestampOutputString),
+ builder.AppendFormat(CultureInfo.InvariantCulture, "{0} {1}{2} = {3}\n",
+ timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture),
definition.Name, labelText, aggregator.Snapshot());
private static void DefaultAppend(
StringBuilder builder,
MetricDefinition definition,
string labelText,
MetricAggregator<long> aggregator,
DateTimeOffset timestamp,
string timestampOutputString) =>
- builder.AppendFormat("{0} {1}{2} = {{{3}}}\n", timestamp.ToString(timestampOutputString),
+ builder.AppendFormat(CultureInfo.InvariantCulture, "{0} {1}{2} = {3}\n",
+ timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture),
definition.Name, labelText, aggregator.Snapshot());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/CommonLib/Services/MetricWriter.cs` around lines 92 - 110, The two
DefaultAppend overloads (MetricWriter.DefaultAppend for MetricAggregator<double>
and MetricAggregator<long>) currently emit numeric values wrapped in literal
braces and use culture-sensitive formatting; update both to remove the extra
braces around the value placeholder so the format string renders the raw numeric
value, and format both the timestamp and the aggregator snapshot using
CultureInfo.InvariantCulture (e.g., call
timestamp.ToString(timestampOutputString, CultureInfo.InvariantCulture) and
convert the aggregator.Snapshot() to a string with InvariantCulture) so output
is locale-independent and produces " = value" instead of " = {value}".
| foreach (var t in observations) { | ||
| Assert.InRange(t, 0.0, 1000.1); | ||
| } |
There was a problem hiding this comment.
Range-only loops are vacuous when no observations are recorded
These assertions pass if the bags are empty, so callback regressions can slip through undetected (especially in parallel/timeout paths). Add count assertions for the expected successful observations before value-range checks.
Suggested test hardening
var adaptiveTimeoutResult = adaptiveTimeout.GetAdaptiveTimeout();
Assert.Equal(maxTimeout, adaptiveTimeoutResult);
+ Assert.Equal(numSamples + 3, observations.Count);
foreach (var t in observations) {
Assert.InRange(t, 0.0, 1000.1);
} Assert.True(adaptiveTimeoutResult < maxTimeout);
+ Assert.Equal(numSamples + 4, completedObservations.Count);
foreach (var t in completedObservations) {
Assert.InRange(t, 0.0, 50.0);
}
foreach (var t in timeoutObservations) {
Assert.InRange(t, 0.0, 1000.1);
}Also applies to: 155-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unit/AdaptiveTimeoutTest.cs` around lines 119 - 121, The range checks
over 'observations' are vacuous when the collection is empty; update the test in
AdaptiveTimeoutTest to first assert the expected count (e.g.,
Assert.True/Assert.Equal on observations.Count or observations.Length) before
iterating, then perform the existing Assert.InRange checks; apply the same fix
to the second occurrence referenced (the block around lines 155-160). Ensure you
reference the 'observations' collection in both test blocks so missing callbacks
fail the count assertion before value checks.
1 participant
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