feat: add support for OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT #3090
Conversation
|
msierks-pcln
force-pushed
the
span-attribute-value-length-limit
branch
2 times, most recently
from
3542d6c to
1bfde50
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3090 +/- ##
=======================================
+ Coverage 80.1% 80.2% +0.1%
=======================================
Files 126 126
Lines 21957 22109 +152
=======================================
+ Hits 17603 17752 +149
- Misses 4354 4357 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
msierks-pcln
force-pushed
the
span-attribute-value-length-limit
branch
2 times, most recently
from
5294944 to
5a73cc1
Compare
opentelemetry/CHANGELOG.md
Outdated
| @@ -4,6 +4,7 @@ | |||
|
|
|||
| - *Breaking* Change return type of `opentelemetry::global::set_tracer_provider` to Unit to align with metrics counterpart | |||
| - Add `get_all` method to `opentelemetry::propagation::Extractor` to return all values of the given propagation key and provide a default implementation. | |||
| - Add support for max span attribute value length; Applies to `Value::String` and `Array::String` only. | |||
There was a problem hiding this comment.
Attribute length enforcements are not part of opentelemetry crate. Its part of opentelemetry_sdk crate, so changelog should be in that crate.
| { | ||
| config.span_limits.max_attribute_value_length = max_attribute_value_length; | ||
| } else if let Some(max_attribute_value_length) = | ||
| env::var("OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT") |
There was a problem hiding this comment.
supporting OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMIT looks reasonable, but OTEL_ATTRIBUTE_VALUE_LENGTH_LIMIT is not something we can add right now, as Logs don't respect this at all.
| @@ -378,6 +378,12 @@ impl TracerProviderBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Specify the maximum allowed length of attribute values. | |||
There was a problem hiding this comment.
could you enhance the doc to make it clear that this is applicable to string values or array of strings.
and also mention that the trimming behavior is simply trimming!
(some people might expect "..." being added as the last 3 characters to indicate trimming has occurred.. But we are not doing that, and there is no way a backend can tell if trimming has occurred. So if a user opts in to this, they should be made aware of the implications)
| @@ -378,6 +378,12 @@ impl TracerProviderBuilder { | |||
| self | |||
| } | |||
|
|
|||
| /// Specify the maximum allowed length of attribute values. | |||
| pub fn with_max_attribute_value_length(mut self, max_attribute_value_length: i32) -> Self { | |||
There was a problem hiding this comment.
we expect positive values only right? i32 is probably the wrong choice.
opentelemetry-sdk/src/trace/span.rs
Outdated
| span.set_attribute(KeyValue::new("third", "test data, after span builder")); | ||
|
|
||
| span.with_data(|data| { | ||
| assert_eq!( |
There was a problem hiding this comment.
instead of picking attributes at each index(0,1,..), could we do a general validation - for each string or array of string, validate that length is <max_attribute_value_length?
| @@ -73,6 +73,14 @@ impl SdkTracer { | |||
| .saturating_sub(span_attributes_limit); | |||
| attribute_options.truncate(span_attributes_limit); | |||
| let dropped_attributes_count = dropped_attributes_count as u32; | |||
| let span_attribute_value_limit = span_limits.max_attribute_value_length; | |||
There was a problem hiding this comment.
set_attribute already does the truncation...Is it needed to be done again on this method?
There was a problem hiding this comment.
From what I can tell, this method is used by the SpanBuilder when a span is initially built. The set_attribute method is used for setting single attributes after the span has been created.
There was a problem hiding this comment.
Looks reasonable. left some small comments.
One is blocking - https://github.com/open-telemetry/opentelemetry-rust/pull/3090/files#r2237524061
msierks-pcln
force-pushed
the
span-attribute-value-length-limit
branch
from
5a73cc1 to
f7dc9cd
Compare
|
@msierks-pcln Can you sign the CLA, so we can consider contributions? It's a one-time process and is mandatory. If contributing on behalf of your employer, please do consult them before signing. |
User description
Fixes #2214
Changes
Add support for the
OTEL_SPAN_ATTRIBUTE_VALUE_LENGTH_LIMITenvironment variable as explained in Opentelemetry SDK environment variable doc. This will truncateValue::StringandArray::Stringattribute values if they exceed the limit.TracerProviderBuilder::with_max_attribute_value_length()Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes