-
Notifications
You must be signed in to change notification settings - Fork 775
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
Changes from 8 commits
d6d9b60
7774f61
351cc6c
dd60d42
93617c5
cc6c278
f9587b9
7bd16ce
6df3400
44dcc61
87a0b67
830b79f
6c10f4d
b7009fa
ac6a7c6
aa0d210
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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 streams, | ||||||
/// so this control the number of streams. | ||||||
/// </summary> | ||||||
/// <param name="meterProviderBuilder">MeterProviderBuilder instance.</param> | ||||||
/// <param name="maxMetricStreams">Maximum 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 SetMaxMetricStreams(this MeterProviderBuilder meterProviderBuilder, int maxMetricStreams) | ||||||
{ | ||||||
if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase) | ||||||
{ | ||||||
meterProviderBuilderBase.SetMaxMetricStreams(maxMetricStreams); | ||||||
} | ||||||
|
||||||
return meterProviderBuilder; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Sets the maximum number of MetricPoints allowed per metric stream. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I think the name of the method is fine There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maxTimeseries seems a good option. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check this out https://en.wikipedia.org/wiki/Cardinality There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets me proceed as is now, to unblock release. |
||||||
/// This limits the number of unique combinations of key/value pairs used | ||||||
/// for reporting measurements. | ||||||
/// </summary> | ||||||
/// <param name="meterProviderBuilder">MeterProviderBuilder instance.</param> | ||||||
/// <param name="maxMetricPointsPerMetricStream">Maximum 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 at least once, | ||||||
/// 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 SetMaxMetricPointsPerMetricStream(this MeterProviderBuilder meterProviderBuilder, int maxMetricPointsPerMetricStream) | ||||||
{ | ||||||
if (meterProviderBuilder is MeterProviderBuilderBase meterProviderBuilderBase) | ||||||
{ | ||||||
meterProviderBuilderBase.SetMaxMetricPointsPerMetricStream(maxMetricPointsPerMetricStream); | ||||||
} | ||||||
|
||||||
return meterProviderBuilder; | ||||||
} | ||||||
|
||||||
/// <summary> | ||||||
/// Sets the <see cref="ResourceBuilder"/> from which the Resource associated with | ||||||
/// this provider is built from. Overwrites currently set ResourceBuilder. | ||||||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That doesn't seem to be mathematically correct? (I guess we should throw if the provided value is 0).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.