Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add max_attributes_per_event and max_attributes_per_link. #521

Merged
merged 8 commits into from
Apr 21, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions opentelemetry/src/sdk/trace/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
//!
//! Configuration represents the global tracing configuration, overrides
//! can be set for the default OpenTelemetry limits and Sampler.
use crate::sdk::trace::span_limit::SpanLimits;
use crate::{sdk, sdk::trace::Sampler, trace::IdGenerator};
use std::env;
use std::str::FromStr;
Expand All @@ -19,12 +20,8 @@ pub struct Config {
pub sampler: Box<dyn sdk::trace::ShouldSample>,
/// The id generator that the sdk should use
pub id_generator: Box<dyn IdGenerator>,
/// The max events that can be added to a `Span`.
pub max_events_per_span: u32,
/// The max attributes that can be added to a `Span`.
pub max_attributes_per_span: u32,
/// The max links that can be added to a `Span`.
pub max_links_per_span: u32,
/// span limit
pub span_limit: SpanLimits,
/// Contains attributes representing an entity that produces telemetry.
pub resource: Option<Arc<sdk::Resource>>,
}
Expand All @@ -44,19 +41,37 @@ impl Config {

/// Specify the number of events to be recorded per span.
pub fn with_max_events_per_span(mut self, max_events: u32) -> Self {
self.max_events_per_span = max_events;
self.span_limit.max_events_per_span = max_events;
self
}

/// Specify the number of attributes to be recorded per span.
pub fn with_max_attributes_per_span(mut self, max_attributes: u32) -> Self {
self.max_attributes_per_span = max_attributes;
self.span_limit.max_attributes_per_span = max_attributes;
self
}

/// Specify the number of events to be recorded per span.
pub fn with_max_links_per_span(mut self, max_links: u32) -> Self {
self.max_links_per_span = max_links;
self.span_limit.max_links_per_span = max_links;
self
}

/// Specify the number of attributes one event can have.
pub fn with_max_attributes_per_event(mut self, max_attributes: u32) -> Self {
self.span_limit.max_attributes_per_event = max_attributes;
self
}

/// Specify the number of attributes one link can have.
pub fn with_max_attributes_per_link(mut self, max_attributes: u32) -> Self {
self.span_limit.max_attributes_per_link = max_attributes;
self
}

/// Specify all limit via the span_limit
pub fn with_span_limit(mut self, span_limit: SpanLimits) -> Self {
self.span_limit = span_limit;
self
}

Expand All @@ -73,31 +88,29 @@ impl Default for Config {
let mut config = Config {
sampler: Box::new(Sampler::ParentBased(Box::new(Sampler::AlwaysOn))),
id_generator: Box::new(sdk::trace::IdGenerator::default()),
max_events_per_span: 128,
max_attributes_per_span: 128,
max_links_per_span: 128,
span_limit: SpanLimits::default(),
resource: None,
};

if let Some(max_attributes_per_span) = env::var("OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT")
.ok()
.and_then(|count_limit| u32::from_str(&count_limit).ok())
{
config.max_attributes_per_span = max_attributes_per_span;
config.span_limit.max_attributes_per_span = max_attributes_per_span;
}

if let Some(max_events_per_span) = env::var("OTEL_SPAN_EVENT_COUNT_LIMIT")
.ok()
.and_then(|max_events| u32::from_str(&max_events).ok())
{
config.max_events_per_span = max_events_per_span;
config.span_limit.max_events_per_span = max_events_per_span;
}

if let Some(max_links_per_span) = env::var("OTEL_SPAN_LINK_COUNT_LIMIT")
.ok()
.and_then(|max_links| u32::from_str(&max_links).ok())
{
config.max_links_per_span = max_links_per_span;
config.span_limit.max_links_per_span = max_links_per_span;
}

config
Expand Down
2 changes: 2 additions & 0 deletions opentelemetry/src/sdk/trace/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ mod id_generator;
mod provider;
mod sampler;
mod span;
mod span_limit;
mod span_processor;
mod tracer;

Expand All @@ -23,6 +24,7 @@ pub use id_generator::{aws::XrayIdGenerator, IdGenerator};
pub use provider::{Builder, TracerProvider};
pub use sampler::{Sampler, SamplingDecision, SamplingResult, ShouldSample};
pub use span::Span;
pub use span_limit::SpanLimits;
pub use span_processor::{
BatchConfig, BatchSpanProcessor, BatchSpanProcessorBuilder, SimpleSpanProcessor, SpanProcessor,
};
Expand Down
111 changes: 104 additions & 7 deletions opentelemetry/src/sdk/trace/span.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
//! start time is set to the current time on span creation. After the `Span` is created, it
//! is possible to change its name, set its `Attributes`, and add `Links` and `Events`.
//! These cannot be changed after the `Span`'s end time has been set.
use crate::sdk::trace::SpanLimits;
use crate::trace::{Event, SpanContext, SpanId, SpanKind, StatusCode};
use crate::{sdk, trace, KeyValue};
use std::borrow::Cow;
Expand All @@ -20,6 +21,7 @@ pub struct Span {
span_context: SpanContext,
data: Option<SpanData>,
tracer: sdk::trace::Tracer,
span_limit: SpanLimits,
}

#[derive(Clone, Debug, PartialEq)]
Expand Down Expand Up @@ -51,11 +53,19 @@ impl Span {
span_context: SpanContext,
data: Option<SpanData>,
tracer: sdk::trace::Tracer,
span_limit: Option<SpanLimits>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be optional? Could switch tests to use Default::default() instead of None

Copy link
Contributor Author

@TommyCpp TommyCpp Apr 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the span_limit is None. Then we will try to get the span limit from the tracer provider. If there isn't a tracer provider connected with the tracer, we will use the default.

Sometimes we may change the span_limit during the span building(For example, we may record the number of dropped entities in the future) so we may pass modified span_limit. But we want to keep the span limit the same as the tracer provider, we can pass None.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm yeah we should store them somewhere. Maybe best path is to have SpanLimits not be optional here and add dropped counts to SpanData? keeps things simple and tracks attribute drops without having to upgrade the provider.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable. I removed the option around SpanLimits. I probably would open another PR to address how to record and expose the number of dropped entities.

) -> Self {
let span_limit = span_limit.unwrap_or_else(|| {
tracer
.provider()
.map(|provider| provider.config().span_limit)
.unwrap_or_default()
});
Span {
span_context,
data,
tracer,
span_limit,
}
}

Expand All @@ -78,9 +88,14 @@ impl crate::trace::Span for Span {
&mut self,
name: String,
timestamp: SystemTime,
attributes: Vec<KeyValue>,
mut attributes: Vec<KeyValue>,
) {
let max_attributes_per_event = self.span_limit.max_attributes_per_event as usize;
self.with_data(|data| {
if attributes.len() > max_attributes_per_event {
attributes.truncate(max_attributes_per_event);
}

data.message_events
.push_back(Event::new(name, timestamp, attributes))
});
Expand Down Expand Up @@ -204,6 +219,10 @@ fn build_export_data(
#[cfg(test)]
mod tests {
use super::*;
use crate::sdk::trace::span_limit::{
DEFAULT_MAX_ATTRIBUTES_PER_EVENT, DEFAULT_MAX_ATTRIBUTES_PER_LINK,
};
use crate::trace::{Link, NoopSpanExporter, TraceId, Tracer};
use crate::{core::KeyValue, trace::Span as _, trace::TracerProvider};
use std::time::Duration;

Expand All @@ -217,9 +236,12 @@ mod tests {
name: "opentelemetry".into(),
start_time: crate::time::now(),
end_time: crate::time::now(),
attributes: sdk::trace::EvictedHashMap::new(config.max_attributes_per_span, 0),
message_events: sdk::trace::EvictedQueue::new(config.max_events_per_span),
links: sdk::trace::EvictedQueue::new(config.max_links_per_span),
attributes: sdk::trace::EvictedHashMap::new(
config.span_limit.max_attributes_per_span,
0,
),
message_events: sdk::trace::EvictedQueue::new(config.span_limit.max_events_per_span),
links: sdk::trace::EvictedQueue::new(config.span_limit.max_links_per_span),
status_code: StatusCode::Unset,
status_message: "".into(),
};
Expand All @@ -228,20 +250,25 @@ mod tests {

fn create_span() -> Span {
let (tracer, data) = init();
Span::new(SpanContext::empty_context(), Some(data), tracer)
Span::new(SpanContext::empty_context(), Some(data), tracer, None)
}

#[test]
fn create_span_without_data() {
let (tracer, _) = init();
let mut span = Span::new(SpanContext::empty_context(), None, tracer);
let mut span = Span::new(SpanContext::empty_context(), None, tracer, None);
span.with_data(|_data| panic!("there are data"));
}

#[test]
fn create_span_with_data_mut() {
let (tracer, data) = init();
let mut span = Span::new(SpanContext::empty_context(), Some(data.clone()), tracer);
let mut span = Span::new(
SpanContext::empty_context(),
Some(data.clone()),
tracer,
None,
);
span.with_data(|d| assert_eq!(*d, data));
}

Expand Down Expand Up @@ -445,4 +472,74 @@ mod tests {
span.end();
assert!(!span.is_recording());
}

#[test]
fn exceed_event_attributes_limit() {
let exporter = NoopSpanExporter::new();
let provider_builder = sdk::trace::TracerProvider::builder().with_simple_exporter(exporter);
let provider = provider_builder.build();
let tracer = provider.get_tracer("opentelemetry-test", None);

let mut event1 = Event::with_name("test event");
for i in 0..(DEFAULT_MAX_ATTRIBUTES_PER_EVENT * 2) {
event1
.attributes
.push(KeyValue::new(format!("key {}", i), i.to_string()))
}
let event2 = event1.clone();

// add event when build
let span_builder = tracer
.span_builder("test")
.with_message_events(vec![event1]);
let mut span = tracer.build(span_builder);

// add event after build
span.add_event("another test event".into(), event2.attributes);

let event_queue = span
.data
.clone()
.expect("span data should not be empty as we already set it before")
.message_events;
let event_vec: Vec<_> = event_queue.iter().take(2).collect();
let processed_event_1 = event_vec.get(0).expect("should have at least two events");
let processed_event_2 = event_vec.get(1).expect("should have at least two events");
assert_eq!(processed_event_1.attributes.len(), 128);
assert_eq!(processed_event_2.attributes.len(), 128);
}

#[test]
fn exceed_link_attributes_limit() {
let exporter = NoopSpanExporter::new();
let provider_builder = sdk::trace::TracerProvider::builder().with_simple_exporter(exporter);
let provider = provider_builder.build();
let tracer = provider.get_tracer("opentelemetry-test", None);

let mut link = Link::new(
SpanContext::new(
TraceId::from_u128(0),
SpanId::from_u64(0),
0,
false,
Default::default(),
),
Vec::new(),
);
for i in 0..(DEFAULT_MAX_ATTRIBUTES_PER_LINK * 2) {
link.attributes_mut()
.push(KeyValue::new(format!("key {}", i), i.to_string()));
}

let span_builder = tracer.span_builder("test").with_links(vec![link]);
let span = tracer.build(span_builder);
let link_queue = span
.data
.clone()
.expect("span data should not be empty as we already set it before")
.links;
let link_vec: Vec<_> = link_queue.iter().collect();
let processed_link = link_vec.get(0).expect("should have at least one link");
assert_eq!(processed_link.attributes().len(), 128);
}
}
47 changes: 47 additions & 0 deletions opentelemetry/src/sdk/trace/span_limit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/// # Span limit
/// Erroneous code can add unintended attributes, events, and links to a span. If these collections
/// are unbounded, they can quickly exhaust available memory, resulting in crashes that are
/// difficult to recover from safely.
///
/// To protected against those errors. Users can use span limit to configure
/// - Maximum allowed span attribute count
/// - Maximum allowed span event count
/// - Maximum allowed span link count
/// - Maximum allowed attribute per span event count
/// - Maximum allowed attribute per span link count
///
/// If the limit has been breached. The attributes, events or links will be dropped based on their
/// index in the collection. The one added to collections later will be dropped first.

pub(crate) const DEFAULT_MAX_EVENT_PER_SPAN: u32 = 128;
pub(crate) const DEFAULT_MAX_ATTRIBUTES_PER_SPAN: u32 = 128;
pub(crate) const DEFAULT_MAX_LINKS_PER_SPAN: u32 = 128;
pub(crate) const DEFAULT_MAX_ATTRIBUTES_PER_EVENT: u32 = 128;
pub(crate) const DEFAULT_MAX_ATTRIBUTES_PER_LINK: u32 = 128;

/// Span limit configuration to keep attributes, events and links to a span in a reasonable number.
#[derive(Copy, Clone, Debug)]
pub struct SpanLimits {
/// The max events that can be added to a `Span`.
pub max_events_per_span: u32,
/// The max attributes that can be added to a `Span`.
pub max_attributes_per_span: u32,
/// The max links that can be added to a `Span`.
pub max_links_per_span: u32,
/// The max attributes that can be added into an `Event`
pub max_attributes_per_event: u32,
/// The max attributes that can be added into a `Link`
pub max_attributes_per_link: u32,
}

impl Default for SpanLimits {
fn default() -> Self {
SpanLimits {
max_events_per_span: DEFAULT_MAX_EVENT_PER_SPAN,
max_attributes_per_span: DEFAULT_MAX_ATTRIBUTES_PER_SPAN,
max_links_per_span: DEFAULT_MAX_LINKS_PER_SPAN,
max_attributes_per_link: DEFAULT_MAX_ATTRIBUTES_PER_LINK,
max_attributes_per_event: DEFAULT_MAX_ATTRIBUTES_PER_EVENT,
}
}
}
Loading