From f1a1835479b55f88f2e81f0e05ec848e6fea440b Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 9 Feb 2024 13:09:56 -0800 Subject: [PATCH] [sdk-metrics] Remove some reflection used by tests (#5338) Co-authored-by: Utkarsh Umesan Pillai <66651184+utpilla@users.noreply.github.com> --- src/OpenTelemetry/Metrics/AggregatorStore.cs | 67 +++++++++---------- .../Metrics/MetricPointReclaimTestsBase.cs | 10 +-- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/src/OpenTelemetry/Metrics/AggregatorStore.cs b/src/OpenTelemetry/Metrics/AggregatorStore.cs index bccdbbf42c7..a5d603589ad 100644 --- a/src/OpenTelemetry/Metrics/AggregatorStore.cs +++ b/src/OpenTelemetry/Metrics/AggregatorStore.cs @@ -15,6 +15,7 @@ internal sealed class AggregatorStore internal readonly bool OutputDeltaWithUnusedMetricPointReclaimEnabled; internal readonly int CardinalityLimit; internal readonly bool EmitOverflowAttribute; + internal readonly ConcurrentDictionary? TagsToMetricPointIndexDictionaryDelta; internal long DroppedMeasurements = 0; private static readonly string MetricPointCapHitFixMessage = "Consider opting in for the experimental SDK feature to emit all the throttled metrics under the overflow attribute by setting env variable OTEL_DOTNET_EXPERIMENTAL_METRICS_EMIT_OVERFLOW_ATTRIBUTE = true. You could also modify instrumentation to reduce the number of unique key/value pair combinations. Or use Views to drop unwanted tags. Or use MeterProviderBuilder.SetMaxMetricPointsPerMetricStream to set higher limit."; @@ -32,8 +33,6 @@ internal sealed class AggregatorStore private readonly ConcurrentDictionary tagsToMetricPointIndexDictionary = new(); - private readonly ConcurrentDictionary? tagsToMetricPointIndexDictionaryDelta; - private readonly string name; private readonly string metricPointCapHitMessage; private readonly MetricPoint[] metricPoints; @@ -110,7 +109,7 @@ internal AggregatorStore( // There is no overload which only takes capacity as the parameter // Using the DefaultConcurrencyLevel defined in the ConcurrentDictionary class: https://github.com/dotnet/runtime/blob/v7.0.5/src/libraries/System.Collections.Concurrent/src/System/Collections/Concurrent/ConcurrentDictionary.cs#L2020 // We expect at the most (maxMetricPoints - reservedMetricPointsCount) * 2 entries- one for sorted and one for unsorted input - this.tagsToMetricPointIndexDictionaryDelta = + this.TagsToMetricPointIndexDictionaryDelta = new ConcurrentDictionary(concurrencyLevel: Environment.ProcessorCount, capacity: (cardinalityLimit - reservedMetricPointsCount) * 2); // Add all the indices except for the reserved ones to the queue so that threads have @@ -266,28 +265,28 @@ internal void SnapshotDeltaWithMetricPointReclaim() // Snapshot method can use this to skip trying to reclaim indices which have already been reclaimed and added to the queue. metricPoint.LookupData = null; - Debug.Assert(this.tagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null"); + Debug.Assert(this.TagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null"); - lock (this.tagsToMetricPointIndexDictionaryDelta!) + lock (this.TagsToMetricPointIndexDictionaryDelta!) { LookupData? dictionaryValue; if (lookupData.SortedTags != Tags.EmptyTags) { // Check if no other thread added a new entry for the same Tags. // If no, then remove the existing entries. - if (this.tagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue) && + if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue) && dictionaryValue == lookupData) { - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out var _); - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out var _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _); } } else { - if (this.tagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue) && + if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue) && dictionaryValue == lookupData) { - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out var _); } } @@ -550,11 +549,11 @@ private int LookupAggregatorStoreForDeltaWithReclaim(KeyValuePair 1) { @@ -567,7 +566,7 @@ private int LookupAggregatorStoreForDeltaWithReclaim(KeyValuePair 0) @@ -612,8 +611,8 @@ private int LookupAggregatorStoreForDeltaWithReclaim(KeyValuePair 0) @@ -658,7 +657,7 @@ private int LookupAggregatorStoreForDeltaWithReclaim(KeyValuePair 1) { // check again after acquiring lock. - if (!this.tagsToMetricPointIndexDictionaryDelta!.TryGetValue(givenTags, out lookupData) && - !this.tagsToMetricPointIndexDictionaryDelta.TryGetValue(sortedTags, out lookupData)) + if (!this.TagsToMetricPointIndexDictionaryDelta!.TryGetValue(givenTags, out lookupData) && + !this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(sortedTags, out lookupData)) { // Check for an available MetricPoint if (this.availableMetricPoints!.Count > 0) @@ -769,14 +768,14 @@ private bool TryGetAvailableMetricPointRare( // MetricPoint, if dictionary entry found. // Add the sorted order along with the given order of tags - this.tagsToMetricPointIndexDictionaryDelta.TryAdd(sortedTags, lookupData); - this.tagsToMetricPointIndexDictionaryDelta.TryAdd(givenTags, lookupData); + this.TagsToMetricPointIndexDictionaryDelta.TryAdd(sortedTags, lookupData); + this.TagsToMetricPointIndexDictionaryDelta.TryAdd(givenTags, lookupData); } } else { // check again after acquiring lock. - if (!this.tagsToMetricPointIndexDictionaryDelta!.TryGetValue(givenTags, out lookupData)) + if (!this.TagsToMetricPointIndexDictionaryDelta!.TryGetValue(givenTags, out lookupData)) { // Check for an available MetricPoint if (this.availableMetricPoints!.Count > 0) @@ -800,7 +799,7 @@ private bool TryGetAvailableMetricPointRare( // MetricPoint, if dictionary entry found. // givenTags will always be sorted when tags length == 1 - this.tagsToMetricPointIndexDictionaryDelta.TryAdd(givenTags, lookupData); + this.TagsToMetricPointIndexDictionaryDelta.TryAdd(givenTags, lookupData); } } @@ -823,23 +822,23 @@ private int RemoveStaleEntriesAndGetAvailableMetricPointRare(LookupData lookupDa // If self-claimed, then add a fresh entry to the dictionary // If an available MetricPoint is found, then only increment the ReferenceCount - Debug.Assert(this.tagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null"); + Debug.Assert(this.TagsToMetricPointIndexDictionaryDelta != null, "this.tagsToMetricPointIndexDictionaryDelta was null"); // Delete the entry for these Tags and get another MetricPoint. - lock (this.tagsToMetricPointIndexDictionaryDelta!) + lock (this.TagsToMetricPointIndexDictionaryDelta!) { LookupData? dictionaryValue; if (lookupData.SortedTags != Tags.EmptyTags) { // Check if no other thread added a new entry for the same Tags in the meantime. // If no, then remove the existing entries. - if (this.tagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue)) + if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.SortedTags, out dictionaryValue)) { if (dictionaryValue == lookupData) { // No other thread added a new entry for the same Tags. - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out _); - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.SortedTags, out _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out _); } else { @@ -851,12 +850,12 @@ private int RemoveStaleEntriesAndGetAvailableMetricPointRare(LookupData lookupDa } else { - if (this.tagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue)) + if (this.TagsToMetricPointIndexDictionaryDelta.TryGetValue(lookupData.GivenTags, out dictionaryValue)) { if (dictionaryValue == lookupData) { // No other thread added a new entry for the same Tags. - this.tagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out _); + this.TagsToMetricPointIndexDictionaryDelta.TryRemove(lookupData.GivenTags, out _); } else { diff --git a/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs b/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs index de5eb88a0af..a10849c4057 100644 --- a/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs +++ b/test/OpenTelemetry.Tests/Metrics/MetricPointReclaimTestsBase.cs @@ -1,9 +1,7 @@ // Copyright The OpenTelemetry Authors // SPDX-License-Identifier: Apache-2.0 -using System.Collections.Concurrent; using System.Diagnostics.Metrics; -using System.Reflection; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using OpenTelemetry.Tests; @@ -286,14 +284,9 @@ private sealed class CustomExporter : BaseExporter private readonly bool assertNoDroppedMeasurements; - private readonly FieldInfo metricPointLookupDictionaryFieldInfo; - public CustomExporter(bool assertNoDroppedMeasurements) { this.assertNoDroppedMeasurements = assertNoDroppedMeasurements; - - var aggregatorStoreFields = typeof(AggregatorStore).GetFields(BindingFlags.NonPublic | BindingFlags.Instance); - this.metricPointLookupDictionaryFieldInfo = aggregatorStoreFields!.FirstOrDefault(field => field.Name == "tagsToMetricPointIndexDictionaryDelta"); } public override ExportResult Export(in Batch batch) @@ -301,8 +294,7 @@ public override ExportResult Export(in Batch batch) foreach (var metric in batch) { var aggStore = metric.AggregatorStore; - var metricPointLookupDictionary = this.metricPointLookupDictionaryFieldInfo.GetValue(aggStore) as ConcurrentDictionary; - + var metricPointLookupDictionary = aggStore.TagsToMetricPointIndexDictionaryDelta; var droppedMeasurements = aggStore.DroppedMeasurements; if (this.assertNoDroppedMeasurements)