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

Allow ability to override max Metric streams and MetricPoints per stream #2635

Merged
Merged
14 changes: 8 additions & 6 deletions src/OpenTelemetry/Metrics/AggregatorStore.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ namespace OpenTelemetry.Metrics
{
internal sealed class AggregatorStore
{
internal const int MaxMetricPoints = 2000;
private static readonly ObjectArrayEqualityComparer ObjectArrayComparer = new ObjectArrayEqualityComparer();
private readonly object lockZeroTags = new object();
private readonly HashSet<string> tagKeysInteresting;
Expand All @@ -42,6 +41,7 @@ internal sealed class AggregatorStore
private readonly double[] histogramBounds;
private readonly UpdateLongDelegate updateLongCallback;
private readonly UpdateDoubleDelegate updateDoubleCallback;
private readonly int metricPointLimit;
private int metricPointIndex = 0;
private int batchSize = 0;
private bool zeroTagMetricPointInitialized;
Expand All @@ -51,11 +51,13 @@ internal sealed class AggregatorStore
internal AggregatorStore(
AggregationType aggType,
AggregationTemporality temporality,
int metricPointLimit,
double[] histogramBounds,
string[] tagKeysInteresting = null)
{
this.metricPoints = new MetricPoint[MaxMetricPoints];
this.currentMetricPointBatch = new int[MaxMetricPoints];
this.metricPointLimit = metricPointLimit;
this.metricPoints = new MetricPoint[metricPointLimit];
Copy link
Member Author

Choose a reason for hiding this comment

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

I think metricPoinLimit+1 should be used, as we reserve one point for zero dimension case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it not be misleading for the user if we add +1 here? Maybe we could update the remarks for the public method SetMetricPointPerMetricStreamLimit in that case?

Copy link
Member

@reyang reyang Nov 18, 2021

Choose a reason for hiding this comment

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

I think metricPoinLimit+1 should be used, as we reserve one point for zero dimension case.

That doesn't seem to be mathematically correct? (I guess we should throw if the provided value is 0).

Copy link
Contributor

Choose a reason for hiding this comment

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

We would throw if the provided value is less than 1 as the method validates the input:
Guard.Range(metricPointLimit, min: 1);

I think what could be confusing is that the user provides a value thinking that it will be maximum number of time-series allowed but internally we would be allowing an additional time-series.

Copy link
Member

Choose a reason for hiding this comment

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

Should we consider doing away with the reservation of the zero dimension case? I guess I don't know the history of why we do it.

this.currentMetricPointBatch = new int[metricPointLimit];
this.aggType = aggType;
this.temporality = temporality;
this.outputDelta = temporality == AggregationTemporality.Delta ? true : false;
Expand Down Expand Up @@ -98,7 +100,7 @@ internal void Update(double value, ReadOnlySpan<KeyValuePair<string, object>> ta
internal int Snapshot()
{
this.batchSize = 0;
var indexSnapshot = Math.Min(this.metricPointIndex, MaxMetricPoints - 1);
var indexSnapshot = Math.Min(this.metricPointIndex, this.metricPointLimit - 1);
if (this.temporality == AggregationTemporality.Delta)
{
this.SnapshotDelta(indexSnapshot);
Expand Down Expand Up @@ -197,7 +199,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng
if (!value2metrics.TryGetValue(tagValues, out aggregatorIndex))
{
aggregatorIndex = this.metricPointIndex;
if (aggregatorIndex >= MaxMetricPoints)
if (aggregatorIndex >= this.metricPointLimit)
{
// sorry! out of data points.
// TODO: Once we support cleanup of
Expand All @@ -212,7 +214,7 @@ private int LookupAggregatorStore(string[] tagKeys, object[] tagValues, int leng
if (!value2metrics.TryGetValue(tagValues, out aggregatorIndex))
{
aggregatorIndex = Interlocked.Increment(ref this.metricPointIndex);
if (aggregatorIndex >= MaxMetricPoints)
if (aggregatorIndex >= this.metricPointLimit)
{
// sorry! out of data points.
// TODO: Once we support cleanup of
Expand Down
22 changes: 22 additions & 0 deletions src/OpenTelemetry/Metrics/MeterProviderBuilderBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,14 @@ namespace OpenTelemetry.Metrics
/// </summary>
public abstract class MeterProviderBuilderBase : MeterProviderBuilder
{
internal const int MaxMetrics = 1000;
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
internal const int MaxMetricPointsPerMetric = 2000;
private readonly List<InstrumentationFactory> instrumentationFactories = new List<InstrumentationFactory>();
private readonly List<string> meterSources = new List<string>();
private readonly List<Func<Instrument, MetricStreamConfiguration>> viewConfigs = new List<Func<Instrument, MetricStreamConfiguration>>();
private ResourceBuilder resourceBuilder = ResourceBuilder.CreateDefault();
private int metricStreamLimit = MaxMetrics;
private int metricPointLimit = MaxMetricPointsPerMetric;

protected MeterProviderBuilderBase()
{
Expand Down Expand Up @@ -105,6 +109,22 @@ internal MeterProviderBuilder AddView(Func<Instrument, MetricStreamConfiguration
return this;
}

internal MeterProviderBuilder SetMetricStreamLimit(int metricStreamLimit)
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
{
Guard.Range(metricStreamLimit, min: 1);

this.metricStreamLimit = metricStreamLimit;
return this;
}

internal MeterProviderBuilder SetMetricPointPerMetricStreamLimit(int metricPointLimit)
{
Guard.Range(metricPointLimit, min: 1);

this.metricPointLimit = metricPointLimit;
return this;
}

internal MeterProviderBuilder SetResourceBuilder(ResourceBuilder resourceBuilder)
{
Debug.Assert(resourceBuilder != null, $"{nameof(resourceBuilder)} must not be null");
Expand All @@ -124,6 +144,8 @@ protected MeterProvider Build()
this.meterSources,
this.instrumentationFactories,
this.viewConfigs,
this.metricStreamLimit,
this.metricPointLimit,
this.MetricReaders.ToArray());
}

Expand Down
47 changes: 47 additions & 0 deletions src/OpenTelemetry/Metrics/MeterProviderBuilderExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,53 @@ public static MeterProviderBuilder AddView(this MeterProviderBuilder meterProvid
return meterProviderBuilder;
}

/// <summary>
/// Sets the maximum number of Metric streams supported by the MeterProvider.
/// When no Views are configured, every instrument will result in one metric stream,
/// so this control the numbers of instruments supported.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
/// When Views are configued, a single instrument can result in multiple metric stream,
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
/// so this controls the number of streams.
/// </summary>
/// <param name="meterProviderBuilder">MeterProviderBuilder instance.</param>
/// <param name="metricStreamLimit">The number of metric streams allowed.</param>
/// <returns>Returns <see cref="MeterProviderBuilder"/> for chaining.</returns>
/// <remarks>
/// If an instrument is created, but disposed later, this will still be contributing to the limit.
/// This may change in the future.
/// </remarks>
public static MeterProviderBuilder SetMetricStreamLimit(this MeterProviderBuilder meterProviderBuilder, int metricStreamLimit)
{
if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase)
{
meterProviderBuilderBase.SetMetricStreamLimit(metricStreamLimit);
}

return meterProviderBuilder;
}

/// <summary>
/// Sets the maximum number of MetricPoints allowed per metric stream.
Copy link
Member

Choose a reason for hiding this comment

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

Bit of a nit, but this is the first time we're publicly introducing the term MetricPoint. So maybe

Suggested change
/// Sets the maximum number of MetricPoints allowed per metric stream.
/// Sets the maximum number of metric points allowed per metric stream.

I think the name of the method is fine SetMetricPointPerMetricStreamLimit ... unless there's a better term from the spec for describing the MetricPoint concept?

Copy link
Member

Choose a reason for hiding this comment

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

Personal gut feeling - maybe for the user "cardinality" is something easier to digest?

Copy link
Contributor

Choose a reason for hiding this comment

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

How about SetTimeSeriesLimit?

Copy link
Member Author

Choose a reason for hiding this comment

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

maxTimeseries seems a good option.

Copy link
Member Author

Choose a reason for hiding this comment

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

cardinality - i think this would be a better fit, if the limit was for limiting the cardinality of an individual tag.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

lets me proceed as is now, to unblock release.
Will discuss the naming issues in next SIG meeting. We have ~10 more days before planned stable release, for coming up with better names)

/// This limis the number of unique combination of key value pairs used
/// for reporting measurement.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
/// <param name="meterProviderBuilder">MeterProviderBuilder instance.</param>
/// <param name="metricPointLimit">The maximum number of metric points allowed per metric stream.</param>
/// <returns>Returns <see cref="MeterProviderBuilder"/> for chaining.</returns>
/// <remarks>
/// If a particular combination of key value pair is used atleast once,
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
/// it will still be contributing to the limit.
/// This may change in the future.
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
/// </remarks>
public static MeterProviderBuilder SetMetricPointPerMetricStreamLimit(this MeterProviderBuilder meterProviderBuilder, int metricPointLimit)
{
if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase)
{
meterProviderBuilderBase.SetMetricPointPerMetricStreamLimit(metricPointLimit);
}

return meterProviderBuilder;
}

/// <summary>
/// Sets the <see cref="ResourceBuilder"/> from which the Resource associated with
/// this provider is built from. Overwrites currently set ResourceBuilder.
Expand Down
21 changes: 13 additions & 8 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ namespace OpenTelemetry.Metrics
{
internal sealed class MeterProviderSdk : MeterProvider
{
internal const int MaxMetrics = 1000;
internal int ShutdownCount;
private readonly Metric[] metrics;
private readonly Metric[] metricsCurrentBatch;
Expand All @@ -38,6 +37,8 @@ internal sealed class MeterProviderSdk : MeterProvider
private readonly HashSet<string> metricStreamNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
private readonly MeterListener listener;
private readonly MetricReader reader;
private readonly int metricStreamLimit;
private readonly int metricPointLimit;
private int metricIndex = -1;
private bool disposed;

Expand All @@ -46,12 +47,16 @@ internal sealed class MeterProviderSdk : MeterProvider
IEnumerable<string> meterSources,
List<MeterProviderBuilderBase.InstrumentationFactory> instrumentationFactories,
List<Func<Instrument, MetricStreamConfiguration>> viewConfigs,
int metricStreamLimit,
int metricPointLimit,
IEnumerable<MetricReader> readers)
{
this.Resource = resource;
this.viewConfigs = viewConfigs;
this.metrics = new Metric[MaxMetrics];
this.metricsCurrentBatch = new Metric[MaxMetrics];
this.metrics = new Metric[metricStreamLimit];
this.metricsCurrentBatch = new Metric[metricStreamLimit];
this.metricStreamLimit = metricStreamLimit;
this.metricPointLimit = metricPointLimit;

AggregationTemporality temporality = AggregationTemporality.Cumulative;

Expand Down Expand Up @@ -183,7 +188,7 @@ internal sealed class MeterProviderSdk : MeterProvider
}

var index = ++this.metricIndex;
if (index >= MaxMetrics)
if (index >= this.metricStreamLimit)
{
// TODO: Log that instrument is ignored
// as max number of Metrics have reached.
Expand All @@ -195,7 +200,7 @@ internal sealed class MeterProviderSdk : MeterProvider
string[] tagKeysInteresting = metricStreamConfig?.TagKeys;
double[] histogramBucketBounds = (metricStreamConfig is HistogramConfiguration histogramConfig
&& histogramConfig.BucketBounds != null) ? histogramConfig.BucketBounds : null;
metric = new Metric(instrument, temporality, metricStreamName, metricDescription, histogramBucketBounds, tagKeysInteresting);
metric = new Metric(instrument, temporality, metricStreamName, metricDescription, this.metricPointLimit, histogramBucketBounds, tagKeysInteresting);

this.metrics[index] = metric;
metrics.Add(metric);
Expand Down Expand Up @@ -256,14 +261,14 @@ internal sealed class MeterProviderSdk : MeterProvider
}

var index = ++this.metricIndex;
if (index >= MaxMetrics)
if (index >= this.metricStreamLimit)
{
OpenTelemetrySdkEventSource.Log.MetricInstrumentIgnored(metricName, instrument.Meter.Name, "Maximum allowed Metrics for the provider exceeded.", "Use views to drop unused instruments. Or configure Provider to allow higher limit.");
return;
}
else
{
metric = new Metric(instrument, temporality, metricName, instrument.Description);
metric = new Metric(instrument, temporality, metricName, instrument.Description, this.metricPointLimit);
this.metrics[index] = metric;
this.metricStreamNames.Add(metricName);
}
Expand Down Expand Up @@ -437,7 +442,7 @@ internal Batch<Metric> Collect()
OpenTelemetrySdkEventSource.Log.MetricObserverCallbackException(exception);
}

var indexSnapshot = Math.Min(this.metricIndex, MaxMetrics - 1);
var indexSnapshot = Math.Min(this.metricIndex, this.metricStreamLimit - 1);
var target = indexSnapshot + 1;
int metricCountCurrentBatch = 0;
for (int i = 0; i < target; i++)
Expand Down
3 changes: 2 additions & 1 deletion src/OpenTelemetry/Metrics/Metric.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public sealed class Metric
AggregationTemporality temporality,
string metricName,
string metricDescription,
int metricPointLimit,
double[] histogramBounds = null,
string[] tagKeysInteresting = null)
{
Expand Down Expand Up @@ -104,7 +105,7 @@ public sealed class Metric
// TODO: Log and assign some invalid Enum.
}

this.aggStore = new AggregatorStore(aggType, temporality, histogramBounds ?? DefaultHistogramBounds, tagKeysInteresting);
this.aggStore = new AggregatorStore(aggType, temporality, metricPointLimit, histogramBounds ?? DefaultHistogramBounds, tagKeysInteresting);
this.Temporality = temporality;
this.InstrumentDisposed = false;
}
Expand Down
12 changes: 6 additions & 6 deletions test/OpenTelemetry.Tests/Metrics/MetricAPITest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -449,26 +449,26 @@ int MetricPointCount()
// for no tag point!
// This may be changed later.
counterLong.Add(10);
for (int i = 0; i < AggregatorStore.MaxMetricPoints + 1; i++)
for (int i = 0; i < MeterProviderBuilderBase.MaxMetricPointsPerMetric + 1; i++)
{
counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
}

metricReader.Collect();
Assert.Equal(AggregatorStore.MaxMetricPoints, MetricPointCount());
Assert.Equal(MeterProviderBuilderBase.MaxMetricPointsPerMetric, MetricPointCount());

metricItems.Clear();
counterLong.Add(10);
for (int i = 0; i < AggregatorStore.MaxMetricPoints + 1; i++)
for (int i = 0; i < MeterProviderBuilderBase.MaxMetricPointsPerMetric + 1; i++)
{
counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
}

metricReader.Collect();
Assert.Equal(AggregatorStore.MaxMetricPoints, MetricPointCount());
Assert.Equal(MeterProviderBuilderBase.MaxMetricPointsPerMetric, MetricPointCount());

counterLong.Add(10);
for (int i = 0; i < AggregatorStore.MaxMetricPoints + 1; i++)
for (int i = 0; i < MeterProviderBuilderBase.MaxMetricPointsPerMetric + 1; i++)
{
counterLong.Add(10, new KeyValuePair<string, object>("key", "value" + i));
}
Expand All @@ -479,7 +479,7 @@ int MetricPointCount()
counterLong.Add(10, new KeyValuePair<string, object>("key", "valueC"));
metricItems.Clear();
metricReader.Collect();
Assert.Equal(AggregatorStore.MaxMetricPoints, MetricPointCount());
Assert.Equal(MeterProviderBuilderBase.MaxMetricPointsPerMetric, MetricPointCount());
}

[Fact]
Expand Down