[sdk] Added support for setting CardinalityLimit allowed for the metric managed by the view.
#5312
Conversation
| { | ||
| if (value != null) | ||
| { | ||
| Guard.ThrowIfOutOfRange(value.Value, min: 3, max: int.MaxValue); |
There was a problem hiding this comment.
@Yun-Ting would you put a comment here explaining why min is 3? (consider updating the XML doc to call out the accepted range)
There was a problem hiding this comment.
Yeah, one for the overflowAttribute when emitOverflowAttribute is set to true, and the other for 0 tag, assuming when users set this value, they want to pass in tags.
Currently, the lower limit for .SetMaxMetricPointsPerMetricStream() is 1, though.
In AggregatorStore, we reserved one for the overflowAttribute.
And in doc, it says one should be reserved for 0 tag (no key/value pair associated).
Also, there had been discussions on whether we should reserve 1 for zero dimension: #2635 (comment).
Maybe this topic indeed worth another PR.
Yun-Ting
changed the title
CardinalityLimit allowed for the metric managed by the view.
…/opentelemetry-dotnet into yunl/metricPointPerView
reyang
left a comment
reyang
left a comment
There was a problem hiding this comment.
LGTM. The current test cases are using reflection, I think eventually we should remove reflection and test against the real limits (e.g. set a limit and see if we hit it or not). Not blocking this PR since it requires many changes in other places (including fixing some bugs).
CodeBlanch
added
the
pkg:OpenTelemetry
CodeBlanch
changed the title
CardinalityLimit allowed for the metric managed by the view.CardinalityLimit allowed for the metric managed by the view.
4 participants
Changes
Fixes #5296
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes