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 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 27 additions & 3 deletions opentelemetry/src/sdk/trace/config.rs
Expand Up @@ -7,6 +7,12 @@ use std::env;
use std::str::FromStr;
use std::sync::Arc;

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;

/// Default trace configuration
pub fn config() -> Config {
Config::default()
Expand All @@ -25,6 +31,10 @@ pub struct Config {
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,
/// Contains attributes representing an entity that produces telemetry.
pub resource: Option<Arc<sdk::Resource>>,
}
Expand Down Expand Up @@ -60,6 +70,18 @@ impl Config {
self
}

/// Specify the number of attributes one event can have.
pub fn with_max_attributes_per_event(mut self, max_attributes: u32) -> Self {
self.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.max_attributes_per_link = max_attributes;
self
}

/// Specify the attributes representing the entity that produces telemetry
pub fn with_resource(mut self, resource: sdk::Resource) -> Self {
self.resource = Some(Arc::new(resource));
Expand All @@ -73,9 +95,11 @@ 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,
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,
resource: None,
};

Expand Down
84 changes: 83 additions & 1 deletion opentelemetry/src/sdk/trace/span.rs
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::config::DEFAULT_MAX_ATTRIBUTES_PER_EVENT;
use crate::trace::{Event, SpanContext, SpanId, SpanKind, StatusCode};
use crate::{sdk, trace, KeyValue};
use std::borrow::Cow;
Expand Down Expand Up @@ -78,9 +79,18 @@ 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.tracer
.provider()
.map(|provider| provider.config().max_attributes_per_event)
.unwrap_or(DEFAULT_MAX_ATTRIBUTES_PER_EVENT) as usize;
self.with_data(|data| {
if attributes.len() > max_attributes_per_event {
let _dropped: Vec<_> = attributes.drain((max_attributes_per_event)..).collect();
}
TommyCpp marked this conversation as resolved.
Show resolved Hide resolved

data.message_events
.push_back(Event::new(name, timestamp, attributes))
});
Expand Down Expand Up @@ -204,6 +214,8 @@ fn build_export_data(
#[cfg(test)]
mod tests {
use super::*;
use crate::sdk::trace::config::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 Down Expand Up @@ -445,4 +457,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);
}
}
17 changes: 17 additions & 0 deletions opentelemetry/src/sdk/trace/tracer.rs
Expand Up @@ -259,12 +259,29 @@ impl crate::trace::Tracer for Tracer {
}
let mut links = EvictedQueue::new(config.max_links_per_span);
if let Some(link_options) = &mut link_options {
for link in link_options.iter_mut() {
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to do this? going through append_vec below will call push_back which should already handle the limits configured above

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 I understand correctly, the append_vec only enforces the limit on the number of links. Not the number of attributes of a link

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry I see what's happening nvm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may make sense to redesign how we create a link to enforce the limit when we creating one 🤔 in the future. Gonna have to use EvictedQueue to store the attributes too

Copy link
Member

Choose a reason for hiding this comment

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

Yeah can rethink he we store dropped count and others in a follow up PR perhaps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My current plan is to add fields in SpanLimits to allow us to store the dropped number in it.
Also need to print the warning message when the dropped number reaches some threshold. Cannot really warn whenever we drop because the spec says don't be too noisy on this issue.

// make sure the attributes is less than max_attribute_per_link
let attributes = link.attributes_mut();
if attributes.len() > config.max_attributes_per_link as usize {
let _dropped: Vec<_> = attributes
.drain((config.max_attributes_per_link as usize)..)
.collect();
}
}
links.append_vec(link_options);
}
let start_time = start_time.unwrap_or_else(crate::time::now);
let end_time = end_time.unwrap_or(start_time);
let mut message_events_queue = EvictedQueue::new(config.max_events_per_span);
if let Some(mut events) = message_events {
for event in events.iter_mut() {
let attributes = &mut event.attributes;
if attributes.len() > config.max_attributes_per_event as usize {
let _dropped: Vec<_> = attributes
.drain((config.max_attributes_per_event as usize)..)
.collect();
}
}
message_events_queue.append_vec(&mut events);
}
let status_code = status_code.unwrap_or(StatusCode::Unset);
Expand Down
5 changes: 5 additions & 0 deletions opentelemetry/src/trace/link.rs
Expand Up @@ -30,4 +30,9 @@ impl Link {
pub fn attributes(&self) -> &Vec<KeyValue> {
&self.attributes
}

/// Mutable attributes of the link
pub(crate) fn attributes_mut(&mut self) -> &mut Vec<KeyValue> {
self.attributes.as_mut()
}
}