Span attribute count and length limits #3231
Conversation
| /// <param name="spanLimits">The configured <see cref="SpanLimits" />.</param> | ||
| public static void SetSpanLimits(SpanLimits spanLimits) | ||
| { | ||
| SpanAttributeCountLimit = spanLimits.AttributeCountLimit; |
There was a problem hiding this comment.
How to handle error when the value is out of range?
There was a problem hiding this comment.
Good question. My thought was that if set to negative then it it won't hurt anything - i.e., the setting would have no effect and nothing would be limited. At least that was my intent... though just realized that setting SpanAttributeValueLengthLimit to a negative value will cause an exception upon calling String.Substring. Will fix.
I'm open to other opinions, but since this is part of the API and not the SDK, I figured an error here was not appropriate.
| if (attributeCountLimit-- != 0) | ||
| { | ||
| this.AddInternal(kvp.Key, kvp.Value); | ||
| } |
There was a problem hiding this comment.
Need to exit the loop once the limit is reached (or it will underflow to negative)
There was a problem hiding this comment.
If negative then it will loop over all attributes and then stop. attributeCountLimit is initialized to -1 for this purpose when the user set TracerProvider.SpanAttributeCountLimit = null.
Though, for efficiency, this loop could break when the limit has been reached. Also will add tests to demonstrate when limits are not set.
| /// <summary> | ||
| /// Gets or sets the maximum allowed number of attributes on spans. | ||
| /// </summary> | ||
| public int? AttributeCountLimit { get; set; } = 128; |
There was a problem hiding this comment.
128 is the default identified by the spec. Changing the defaults would be a breaking change, so right now the defaults only apply if someone explicitly calls SetSpanLimits(new SpanLimits()).
…elemetry-dotnet into alanwest/attribute-limits
Codecov Report
@@ Coverage Diff @@
## main #3231 +/- ##
==========================================
+ Coverage 85.55% 85.60% +0.04%
==========================================
Files 261 262 +1
Lines 9390 9420 +30
==========================================
+ Hits 8034 8064 +30
Misses 1356 1356
|
There was a problem hiding this comment.
The actual code logic looks good to me.
I have a design question regarding the goal of these limits. By reading the specification I kind of feel that these limits are intended to provide protection against unbounded memory usage - e.g. if a user keeps adding new attributes, at certain moment the API would become a simple no-op that doesn't has memory cost.
Given .NET is not putting such limits on the Activity APIs, I feel the limits are becoming less useful. Perhaps such logic can be applied to the exporter logic?
Yes, I agree this is one of the primary goals. Another place this concern was raised was in the context of unbounded events being added to a span here #1276 (comment). Another thing this feature offers is support to backends that have their own limitations on the number of attributes, attribute length, etc. that spans can have.
My hope was that .NET would consider implementing configurable limits: dotnet/runtime#68528
I think I'm liking this idea. I'm going to make this PR a draft for now and try this out. |
|
This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day. |
github-actions
bot
added
the
Stale
|
Closing this will follow up with the plan to handle these limits in the exporters. |
Implements attribute limits for span data when using the shim API.
Follow on PR will implement additional span limits for events and links.
I've opened dotnet/runtime#68528 to propose a similar API for
ActivitySource.