OTLP exporter support for limiting activity tags, events, and links #3376
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3376 +/- ##
==========================================
+ Coverage 86.34% 86.40% +0.05%
==========================================
Files 270 271 +1
Lines 9867 9910 +43
==========================================
+ Hits 8520 8563 +43
Misses 1347 1347
|
alanwest
changed the title
|
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
alanwest
removed
the
Stale
|
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
| Assert.Null(config.AttributeValueLengthLimit); | ||
| Assert.Equal(128, config.AttributeCountLimit); | ||
| Assert.Null(config.SpanAttributeValueLengthLimit); | ||
| Assert.Equal(128, config.SpanAttributeCountLimit); | ||
| Assert.Equal(128, config.SpanEventCountLimit); | ||
| Assert.Equal(128, config.SpanLinkCountLimit); | ||
| Assert.Equal(128, config.EventAttributeCountLimit); | ||
| Assert.Equal(128, config.LinkAttributeCountLimit); |
There was a problem hiding this comment.
This is a breaking change. I suppose the specification is a bit ambiguous here https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/sdk-environment-variables.md#attribute-limits. An SDK implementing the environment variables is a SHOULD per the spec, but it's not clear if the default values are also a SHOULD.
We have two options. Make the breaking change and consider this a bug fix - in which case I'll update the changelog to reflect this. Or maintain the prior behavior and not introduce these new defaults.
There was a problem hiding this comment.
I'm going to revert this change to defaults for now so we can come back to this conversation. Setting these new defaults mean everyone takes the perf hit described here #3376 (comment)
| @@ -265,7 +270,7 @@ private static class ActivityTagsEnumeratorFactory<TState> | |||
| private static readonly DictionaryEnumerator<string, object, TState>.ForEachDelegate ForEachTagValueCallbackRef = ForEachTagValueCallback; | |||
|
|
|||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | |||
| public static void Enumerate(Activity activity, ref TState state) | |||
| public static void Enumerate(Activity activity, ref TState state, int? maxTags) | |||
There was a problem hiding this comment.
If we don't go with the overload approach, then maybe assign maxTags a default value of null.
github-actions
bot
removed
the
Stale
Fixes #3297
Related to #1453
This PR adds some support for the attribute limit configuration from the spec:
One reason these limits exist in the spec is to offer users a way to control the memory consumption of instrumentation. This PR does not help with that. In order to control memory consumption, we'd need a way for the API to honor the limits. Currently, we cannot do this. So...
The configuration is only supported by the OTLP exporter for now and therefore the limits only impact the data sent over the wire. This is beneficial for sending data to backends that have their own limitations for number and length of attributes. My plan is to follow up with this support for the other exporters.
Tangential to the primary motivation of this PR, I've introduced an
SdkConfigurationclass. For now, I've dropped it in the OTLP exporter project and kept it internal. Ultimately, I'd like to move this class to the SDK project, but I did not wish to hash out its design and public API as part of this PR. Also, this means the attribute limit configuration can currently only be set via environment variables. Once moved to the SDK programatic configuration would also be possible.