Allow ability to override max Metric streams and MetricPoints per stream #2635
Conversation
cijothomas
changed the title
| this.metricPoints = new MetricPoint[MaxMetricPoints]; | ||
| this.currentMetricPointBatch = new int[MaxMetricPoints]; | ||
| this.metricPointLimit = metricPointLimit; | ||
| this.metricPoints = new MetricPoint[metricPointLimit]; |
There was a problem hiding this comment.
I think metricPoinLimit+1 should be used, as we reserve one point for zero dimension case.
There was a problem hiding this comment.
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.
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).
There was a problem hiding this comment.
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.
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.
Codecov Report
@@ Coverage Diff @@
## main #2635 +/- ##
==========================================
- Coverage 81.37% 81.31% -0.07%
==========================================
Files 245 245
Lines 8614 8645 +31
==========================================
+ Hits 7010 7030 +20
- Misses 1604 1615 +11
|
| } | ||
|
|
||
| /// <summary> | ||
| /// Sets the maximum number of MetricPoints allowed per metric stream. |
There was a problem hiding this comment.
Bit of a nit, but this is the first time we're publicly introducing the term MetricPoint. So maybe
| /// 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?
There was a problem hiding this comment.
Personal gut feeling - maybe for the user "cardinality" is something easier to digest?
There was a problem hiding this comment.
How about SetTimeSeriesLimit?
There was a problem hiding this comment.
maxTimeseries seems a good option.
There was a problem hiding this comment.
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.
Check this out https://en.wikipedia.org/wiki/Cardinality
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
No reason to hold up this PR, but I'm wondering if we should change the name MetricPoint to something else?
The term "metric stream" seems pretty well established in the specification, but "metric point" is not as well established. In fact there is a section of the specification here titled Metric Point, but it refers to a different concept than what we've modeled with the MetricPoint class.
We had used the term DataPoint in the past. Searching that data model specification "data point" is a term that is often used. So, what do folks think about maybe DataPoint or MetricDataPoint?
Co-authored-by: Alan West <3676547+alanwest@users.noreply.github.com>
…/cijothomas/opentelemetry-dotnet into cijothomas/configuremetriclimits
Fixes #2358
This allows user to specify a max limit of number of Metric streams. This is to prevent indefinite memory growth, if a user keeps creating new instruments, instead of reusing for each measurement.
This also allows user to specify a max limit of the number of MetricPoints, per metric stream. This is to prevent indefinite memory usage growth, when high cardinality values are uses in metric tags. This limit is applied independently to every metric stream for this meter provider. In the future, we could add ability to configure separate MaxMetricPoint limit per instrument, by leveraging Views, which can override the
MeterProviderlevel limits.Note that, in the current implementation, MetricPoints are pre-allocated upto max limit when an instrument is created. So user pays upfront memory cost for MaxMetricPoints even though only subset of them is used. In the future, when per-instrument config is allowed, users can tune this number to reduce wastage.
Also as this, is not addressed, there is no ability to "reclaim" a MetricPoint if it has been inactive for some time.
TODO:
Public API
Unit Tests
Decide if the defaults 1000, 2000 are good enough.