Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[repo] Small cleanup/simplification #6048

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static LoggerProviderBuilder AddInstrumentation<

loggerProviderBuilder.ConfigureBuilder((sp, builder) =>
{
builder.AddInstrumentation(() => sp.GetRequiredService<T>());
builder.AddInstrumentation(sp.GetRequiredService<T>);
});

return loggerProviderBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static MeterProviderBuilder AddInstrumentation<

meterProviderBuilder.ConfigureBuilder((sp, builder) =>
{
builder.AddInstrumentation(() => sp.GetRequiredService<T>());
builder.AddInstrumentation(sp.GetRequiredService<T>);
});

return meterProviderBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public static TracerProviderBuilder AddInstrumentation<

tracerProviderBuilder.ConfigureBuilder((sp, builder) =>
{
builder.AddInstrumentation(() => sp.GetRequiredService<T>());
builder.AddInstrumentation(sp.GetRequiredService<T>);
});

return tracerProviderBuilder;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace OpenTelemetry.Internal;
[EventSource(Name = "OpenTelemetry-Api")]
internal sealed class OpenTelemetryApiEventSource : EventSource
{
public static OpenTelemetryApiEventSource Log = new();
public static readonly OpenTelemetryApiEventSource Log = new();

[NonEvent]
public void ActivityContextExtractException(string format, Exception ex)
Expand Down
4 changes: 1 addition & 3 deletions src/OpenTelemetry.Api/Trace/TracerProvider.cs
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ protected override void Dispose(bool disposing)
{
if (disposing)
{
var tracers = Interlocked.CompareExchange(ref this.Tracers, null, this.Tracers);
var tracers = Interlocked.Exchange(ref this.Tracers, null);
if (tracers != null)
{
lock (tracers)
Expand All @@ -90,8 +90,6 @@ protected override void Dispose(bool disposing)
tracer.ActivitySource = null;
activitySource?.Dispose();
}

tracers.Clear();
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace OpenTelemetry.PersistentStorage.Abstractions;
[EventSource(Name = EventSourceName)]
internal sealed class PersistentStorageAbstractionsEventSource : EventSource
{
public static PersistentStorageAbstractionsEventSource Log = new PersistentStorageAbstractionsEventSource();
public static readonly PersistentStorageAbstractionsEventSource Log = new PersistentStorageAbstractionsEventSource();
#if BUILDING_INTERNAL_PERSISTENT_STORAGE
private const string EventSourceName = "OpenTelemetry-PersistentStorage-Abstractions-Otlp";
#else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ namespace OpenTelemetry.PersistentStorage.FileSystem;
[EventSource(Name = EventSourceName)]
internal sealed class PersistentStorageEventSource : EventSource
{
public static PersistentStorageEventSource Log = new PersistentStorageEventSource();
public static readonly PersistentStorageEventSource Log = new PersistentStorageEventSource();
#if BUILDING_INTERNAL_PERSISTENT_STORAGE
private const string EventSourceName = "OpenTelemetry-PersistentStorage-FileSystem-Otlp";
#else
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ namespace OpenTelemetry.Extensions.Hosting.Implementation;
[EventSource(Name = "OpenTelemetry-Extensions-Hosting")]
internal sealed class HostingExtensionsEventSource : EventSource
{
public static HostingExtensionsEventSource Log = new();
public static readonly HostingExtensionsEventSource Log = new();

[Event(1, Message = "OpenTelemetry TracerProvider was not found in application services. Tracing will remain disabled.", Level = EventLevel.Warning)]
public void TracerProviderNotRegistered()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ namespace OpenTelemetry.Internal;
[EventSource(Name = "OpenTelemetry-Sdk")]
internal sealed class OpenTelemetrySdkEventSource : EventSource, IConfigurationExtensionsLogger
{
public static OpenTelemetrySdkEventSource Log = new();
public static readonly OpenTelemetrySdkEventSource Log = new();
#if DEBUG
public static OpenTelemetryEventListener Listener = new();
#endif
Expand Down
6 changes: 2 additions & 4 deletions src/OpenTelemetry/Logs/LogRecord.cs
Original file line number Diff line number Diff line change
Expand Up @@ -429,19 +429,17 @@ public void ForEachScope<TState>(Action<LogRecordScope, TState> callback, TState
{
Guard.ThrowIfNull(callback);

var forEachScopeState = new ScopeForEachState<TState>(callback, state);

var bufferedScopes = this.ILoggerData.BufferedScopes;
if (bufferedScopes != null)
{
foreach (object? scope in bufferedScopes)
{
ScopeForEachState<TState>.ForEachScope(scope, forEachScopeState);
callback(new(scope), state);
}
}
else
{
this.ILoggerData.ScopeProvider?.ForEachScope(ScopeForEachState<TState>.ForEachScope, forEachScopeState);
this.ILoggerData.ScopeProvider?.ForEachScope(ScopeForEachState<TState>.ForEachScope, new(callback, state));
}
}

Expand Down
9 changes: 5 additions & 4 deletions src/OpenTelemetry/Metrics/CircularBufferBuckets.cs
Original file line number Diff line number Diff line change
Expand Up @@ -263,10 +263,11 @@ internal void Reset()
{
if (this.trait != null)
{
for (var i = 0; i < this.trait.Length; ++i)
{
this.trait[i] = 0;
}
#if NET
Array.Clear(this.trait);
#else
Array.Clear(this.trait, 0, this.trait.Length);
#endif
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,12 @@ internal MeterProviderSdk(
}

// Setup Listener
if (state.MeterSources.Any(s => WildcardHelper.ContainsWildcard(s)))
if (state.MeterSources.Exists(WildcardHelper.ContainsWildcard))
{
var regex = WildcardHelper.GetWildcardRegex(state.MeterSources);
this.shouldListenTo = instrument => regex.IsMatch(instrument.Meter.Name);
}
else if (state.MeterSources.Any())
else if (state.MeterSources.Count > 0)
{
var meterSourcesToSubscribe = new HashSet<string>(state.MeterSources, StringComparer.OrdinalIgnoreCase);
this.shouldListenTo = instrument => meterSourcesToSubscribe.Contains(instrument.Meter.Name);
Expand Down
10 changes: 5 additions & 5 deletions src/OpenTelemetry/Metrics/MetricPoint/MetricPointsAccessor.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,9 @@ public readonly struct MetricPointsAccessor
{
private readonly MetricPoint[] metricsPoints;
private readonly int[] metricPointsToProcess;
private readonly long targetCount;
private readonly int targetCount;

internal MetricPointsAccessor(MetricPoint[] metricsPoints, int[] metricPointsToProcess, long targetCount)
internal MetricPointsAccessor(MetricPoint[] metricsPoints, int[] metricPointsToProcess, int targetCount)
{
Debug.Assert(metricsPoints != null, "metricPoints was null");
Debug.Assert(metricPointsToProcess != null, "metricPointsToProcess was null");
Expand All @@ -39,10 +39,10 @@ public struct Enumerator
{
private readonly MetricPoint[] metricsPoints;
private readonly int[] metricPointsToProcess;
private readonly long targetCount;
private long index;
private readonly int targetCount;
private int index;

internal Enumerator(MetricPoint[] metricsPoints, int[] metricPointsToProcess, long targetCount)
internal Enumerator(MetricPoint[] metricsPoints, int[] metricPointsToProcess, int targetCount)
{
this.metricsPoints = metricsPoints;
this.metricPointsToProcess = metricPointsToProcess;
Expand Down
2 changes: 1 addition & 1 deletion src/OpenTelemetry/Metrics/Reader/MetricReader.cs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ public bool Collect(int timeoutMilliseconds = Timeout.Infinite)

if (!shouldRunCollect)
{
return Task.WaitAny(tcs.Task, this.shutdownTcs.Task, Task.Delay(timeoutMilliseconds)) == 0 && tcs.Task.Result;
return Task.WaitAny([tcs.Task, this.shutdownTcs.Task], timeoutMilliseconds) == 0 && tcs.Task.Result;
}

var result = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,7 @@ public class ExplicitBucketHistogramConfiguration : HistogramConfiguration
/// </remarks>
public double[]? Boundaries
{
get
{
if (this.CopiedBoundaries != null)
{
double[] copy = new double[this.CopiedBoundaries.Length];
this.CopiedBoundaries.AsSpan().CopyTo(copy);
return copy;
}

return null;
}
get => this.CopiedBoundaries != null ? this.CopiedBoundaries.AsSpan().ToArray() : null;

set
{
Expand All @@ -46,9 +36,7 @@ public double[]? Boundaries
throw new ArgumentException($"Histogram boundaries are invalid. Histogram boundaries must be in ascending order with distinct values.", nameof(value));
}

double[] copy = new double[value.Length];
value.AsSpan().CopyTo(copy);
this.CopiedBoundaries = copy;
this.CopiedBoundaries = value.AsSpan().ToArray();
}
else
{
Expand Down
27 changes: 2 additions & 25 deletions src/OpenTelemetry/Metrics/View/MetricStreamConfiguration.cs
Original file line number Diff line number Diff line change
Expand Up @@ -71,31 +71,8 @@ public string? Name
/// </remarks>
public string[]? TagKeys
{
get
{
if (this.CopiedTagKeys != null)
{
string[] copy = new string[this.CopiedTagKeys.Length];
this.CopiedTagKeys.AsSpan().CopyTo(copy);
return copy;
}

return null;
}

set
{
if (value != null)
{
string[] copy = new string[value.Length];
value.AsSpan().CopyTo(copy);
this.CopiedTagKeys = copy;
}
else
{
this.CopiedTagKeys = null;
}
}
get => this.CopiedTagKeys != null ? this.CopiedTagKeys.AsSpan().ToArray() : null;
set => this.CopiedTagKeys = value != null ? value.AsSpan().ToArray() : null;
}

/// <summary>
Expand Down
4 changes: 1 addition & 3 deletions src/OpenTelemetry/OpenTelemetrySdkExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ namespace OpenTelemetry;
/// </summary>
public static class OpenTelemetrySdkExtensions
{
private static readonly NullLoggerFactory NoopLoggerFactory = new();

/// <summary>
/// Gets the <see cref="ILoggerFactory"/> contained in an <see
/// cref="OpenTelemetrySdk"/> instance.
Expand All @@ -31,6 +29,6 @@ public static ILoggerFactory GetLoggerFactory(this OpenTelemetrySdk sdk)
Guard.ThrowIfNull(sdk);

return (ILoggerFactory?)sdk.Services.GetService(typeof(ILoggerFactory))
?? NoopLoggerFactory;
?? NullLoggerFactory.Instance;
}
}
5 changes: 4 additions & 1 deletion test/OpenTelemetry.Api.Tests/Trace/TracerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,10 @@ static void InnerTest()
getTracerThread.Join();
}

Assert.Empty(tracers);
foreach (var kvp in tracers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pentp Can you explain why this test is changing? For simple cleanup/simplification work I would expect all the tests to pass as they did before so this worries me a bit 😄

Copy link
Author

@pentp pentp Jan 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's changing because I removed the ConcurrentDictionary.Clear() call here.

It's unnecessary from a functional perspective (the dictionary isn't used in any way after the class field is nulled out) and somewhat expensive - it takes all the locks and allocates new backing arrays which can be potentially quite large: https://source.dot.net/#System.Collections.Concurrent/System/Collections/Concurrent/ConcurrentDictionary.cs,108c257ef528517e

{
Assert.Null(kvp.Value.ActivitySource);
}
}
}

Expand Down