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

Span links stored as Vector instead of EvictedQueue #1313

Merged
merged 23 commits into from Nov 7, 2023
Merged
Show file tree
Hide file tree
Changes from 12 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
5 changes: 3 additions & 2 deletions opentelemetry-contrib/src/trace/exporter/jaeger_json.rs
Expand Up @@ -159,11 +159,12 @@
})
})
.collect::<Vec<_>>();
let mut references = if span.links.is_empty() {
let mut references = if span.span_links.links.is_empty() {

Check warning on line 162 in opentelemetry-contrib/src/trace/exporter/jaeger_json.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-contrib/src/trace/exporter/jaeger_json.rs#L162

Added line #L162 was not covered by tests
None
} else {
Some(
span.links
span.span_links
.links

Check warning on line 167 in opentelemetry-contrib/src/trace/exporter/jaeger_json.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-contrib/src/trace/exporter/jaeger_json.rs#L166-L167

Added lines #L166 - L167 were not covered by tests
.iter()
.map(|link| {
let span_context = &link.span_context;
Expand Down
10 changes: 7 additions & 3 deletions opentelemetry-datadog/src/exporter/model/mod.rs
Expand Up @@ -193,7 +193,11 @@ pub(crate) mod tests {
trace::{SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId, TraceState},
KeyValue,
};
use opentelemetry_sdk::{self, trace::EvictedQueue, InstrumentationLibrary, Resource};
use opentelemetry_sdk::{
self,
trace::{EvictedQueue, SpanLinks},
InstrumentationLibrary, Resource,
};
use std::borrow::Cow;
use std::time::{Duration, SystemTime};

Expand All @@ -216,7 +220,7 @@ pub(crate) mod tests {
let capacity = 3;
let attributes = vec![KeyValue::new("span.type", "web")];
let events = EvictedQueue::new(capacity);
let links = EvictedQueue::new(capacity);
let links = SpanLinks::default();
let resource = Resource::new(vec![KeyValue::new("host.name", "test")]);

trace::SpanData {
Expand All @@ -229,7 +233,7 @@ pub(crate) mod tests {
attributes,
dropped_attributes_count: 0,
events,
links,
span_links: links,
status: Status::Ok,
resource: Cow::Owned(resource),
instrumentation_lib: InstrumentationLibrary::new(
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-jaeger/src/exporter/mod.rs
Expand Up @@ -101,7 +101,7 @@
}
}

fn links_to_references(links: EvictedQueue<Link>) -> Option<Vec<jaeger::SpanRef>> {
fn links_to_references(links: Vec<Link>) -> Option<Vec<jaeger::SpanRef>> {

Check warning on line 104 in opentelemetry-jaeger/src/exporter/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-jaeger/src/exporter/mod.rs#L104

Added line #L104 was not covered by tests
if !links.is_empty() {
let refs = links
.iter()
Expand Down Expand Up @@ -138,7 +138,7 @@
span_id: i64::from_be_bytes(span.span_context.span_id().to_bytes()),
parent_span_id: i64::from_be_bytes(span.parent_span_id.to_bytes()),
operation_name: span.name.into_owned(),
references: links_to_references(span.links),
references: links_to_references(span.span_links.links),

Check warning on line 141 in opentelemetry-jaeger/src/exporter/mod.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-jaeger/src/exporter/mod.rs#L141

Added line #L141 was not covered by tests
flags: span.span_context.trace_flags().to_u8() as i32,
start_time: span
.start_time
Expand Down
18 changes: 14 additions & 4 deletions opentelemetry-proto/src/transform/trace.rs
Expand Up @@ -93,8 +93,13 @@
dropped_attributes_count: event.dropped_attributes_count,
})
.collect(),
dropped_links_count: source_span.links.dropped_count(),
links: source_span.links.into_iter().map(Into::into).collect(),
dropped_links_count: source_span.span_links.dropped_count,
links: source_span
.span_links
.links
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
.into_iter()
.map(Into::into)
.collect(),
status: Some(Status {
code: status::StatusCode::from(&source_span.status).into(),
message: match source_span.status {
Expand Down Expand Up @@ -204,8 +209,13 @@
dropped_attributes_count: event.dropped_attributes_count,
})
.collect(),
dropped_links_count: source_span.links.dropped_count(),
links: source_span.links.into_iter().map(Into::into).collect(),
dropped_links_count: source_span.span_links.dropped_count,
links: source_span
.span_links
.links
.into_iter()
.map(Into::into)
.collect(),

Check warning on line 218 in opentelemetry-proto/src/transform/trace.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/trace.rs#L212-L218

Added lines #L212 - L218 were not covered by tests
status: Some(Status {
code: status::StatusCode::from(&source_span.status).into(),
message: match source_span.status {
Expand Down
24 changes: 21 additions & 3 deletions opentelemetry-proto/src/transform/tracez.rs
Expand Up @@ -19,7 +19,13 @@
endtime: to_nanos(span_data.end_time),
attributes: Attributes::from(span_data.attributes).0,
events: span_data.events.iter().cloned().map(Into::into).collect(),
links: span_data.links.iter().cloned().map(Into::into).collect(),
links: span_data
.span_links
.links
.iter()
.cloned()
.map(Into::into)
.collect(),

Check warning on line 28 in opentelemetry-proto/src/transform/tracez.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/tracez.rs#L22-L28

Added lines #L22 - L28 were not covered by tests
}
}
}
Expand All @@ -33,7 +39,13 @@
starttime: to_nanos(span_data.start_time),
attributes: Attributes::from(span_data.attributes).0,
events: span_data.events.iter().cloned().map(Into::into).collect(),
links: span_data.links.iter().cloned().map(Into::into).collect(),
links: span_data
.span_links
.links
.iter()
.cloned()
.map(Into::into)
.collect(),

Check warning on line 48 in opentelemetry-proto/src/transform/tracez.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/tracez.rs#L42-L48

Added lines #L42 - L48 were not covered by tests
status: match span_data.status {
Status::Error { description } => Some(SpanStatus {
message: description.to_string(),
Expand All @@ -54,7 +66,13 @@
starttime: to_nanos(span_data.start_time),
attributes: Attributes::from(span_data.attributes).0,
events: span_data.events.iter().cloned().map(Into::into).collect(),
links: span_data.links.iter().cloned().map(Into::into).collect(),
links: span_data
.span_links
.links
.iter()
.cloned()
.map(Into::into)
.collect(),

Check warning on line 75 in opentelemetry-proto/src/transform/tracez.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/tracez.rs#L69-L75

Added lines #L69 - L75 were not covered by tests
}
}
}
Expand Down
18 changes: 15 additions & 3 deletions opentelemetry-sdk/CHANGELOG.md
Expand Up @@ -30,8 +30,9 @@
[#1256](https://github.com/open-telemetry/opentelemetry-rust/issues/1256)
- Replace regex with glob (#1301)
- **Breaking**
[#1293](https://github.com/open-telemetry/opentelemetry-rust/issues/1293)
makes few breaking changes with respect to how Span attributes are stored to
[#1293](https://github.com/open-telemetry/opentelemetry-rust/issues/1293),
[#1313](https://github.com/open-telemetry/opentelemetry-rust/issues/1313)
makes few breaking changes with respect to how Span attributes/links are stored to
achieve performance gains. See below for details:

*Behavior Change*:
Expand All @@ -41,11 +42,22 @@
here](https://github.com/open-telemetry/opentelemetry-rust/issues/1300), if
you are affected.

When enforcing `max_attributes_per_span` from `SpanLimits`, attributes are
kept in the first-come order. The previous "eviction" based approach is no
longer performed.

When enforcing `max_links_per_span` from `SpanLimits`, links are kept in the
first-come order. The previous "eviction" based approach is no longer
performed.

*Breaking Change Affecting Exporter authors*:

`SpanData` now stores `attributes` as `Vec<KeyValue>` instead of
`SpanData` now stores `attributes` as `Vec<KeyValue>` instead of
`EvictedHashMap`. `SpanData` now expose `dropped_attributes_count` as a
separate field.

`SpanData` now stores `links` as `SpanLinks` instead of `EvictedQueue`.
`SpanLinks` is a struct with a `Vec` of links and `dropped_count`.

*Breaking Change Affecting Sampler authors*:

Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/benches/batch_span_processor.rs
Expand Up @@ -5,7 +5,7 @@ use opentelemetry::trace::{
use opentelemetry_sdk::export::trace::SpanData;
use opentelemetry_sdk::runtime::Tokio;
use opentelemetry_sdk::testing::trace::NoopSpanExporter;
use opentelemetry_sdk::trace::{BatchSpanProcessor, EvictedQueue, SpanProcessor};
use opentelemetry_sdk::trace::{BatchSpanProcessor, EvictedQueue, SpanLinks, SpanProcessor};
use opentelemetry_sdk::Resource;
use std::borrow::Cow;
use std::sync::Arc;
Expand All @@ -30,7 +30,7 @@ fn get_span_data() -> Vec<SpanData> {
attributes: Vec::new(),
dropped_attributes_count: 0,
events: EvictedQueue::new(12),
links: EvictedQueue::new(12),
span_links: SpanLinks::default(),
status: Status::Unset,
resource: Cow::Owned(Resource::empty()),
instrumentation_lib: Default::default(),
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/src/export/trace.rs
@@ -1,7 +1,7 @@
//! Trace exporters
use crate::Resource;
use futures_util::future::BoxFuture;
use opentelemetry::trace::{Event, Link, SpanContext, SpanId, SpanKind, Status, TraceError};
use opentelemetry::trace::{Event, SpanContext, SpanId, SpanKind, Status, TraceError};
use opentelemetry::KeyValue;
use std::borrow::Cow;
use std::fmt::Debug;
Expand Down Expand Up @@ -89,7 +89,7 @@ pub struct SpanData {
/// Span events
pub events: crate::trace::EvictedQueue<Event>,
/// Span Links
pub links: crate::trace::EvictedQueue<Link>,
pub span_links: crate::trace::SpanLinks,
/// Span status
pub status: Status,
/// Resource contains attributes representing an entity that produced this span.
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-sdk/src/testing/trace/mod.rs
Expand Up @@ -7,7 +7,7 @@ use crate::{
trace::{ExportResult, SpanData, SpanExporter},
ExportError,
},
trace::{Config, EvictedQueue},
trace::{Config, EvictedQueue, SpanLinks},
InstrumentationLibrary,
};
use async_trait::async_trait;
Expand Down Expand Up @@ -37,7 +37,7 @@ pub fn new_test_export_span_data() -> SpanData {
attributes: Vec::new(),
dropped_attributes_count: 0,
events: EvictedQueue::new(config.span_limits.max_events_per_span),
links: EvictedQueue::new(config.span_limits.max_links_per_span),
span_links: SpanLinks::default(),
status: Status::Unset,
resource: config.resource,
instrumentation_lib: InstrumentationLibrary::default(),
Expand Down
11 changes: 11 additions & 0 deletions opentelemetry-sdk/src/trace/links.rs
@@ -0,0 +1,11 @@
//! # Span Links

use opentelemetry::trace::Link;
/// Stores span links along with dropped count.
#[derive(Clone, Debug, Default, PartialEq)]
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
pub struct SpanLinks {
/// The links stored as a vector. Could be empty if there are no links.
pub links: Vec<Link>,
/// The number of links dropped from the span.
pub dropped_count: u32,
}
58 changes: 54 additions & 4 deletions opentelemetry-sdk/src/trace/mod.rs
Expand Up @@ -10,6 +10,7 @@ mod config;
mod evicted_hash_map;
mod evicted_queue;
mod id_generator;
mod links;
mod provider;
mod sampler;
mod span;
Expand All @@ -21,6 +22,7 @@ pub use config::{config, Config};
pub use evicted_hash_map::EvictedHashMap;
pub use evicted_queue::EvictedQueue;
pub use id_generator::{aws::XrayIdGenerator, IdGenerator, RandomIdGenerator};
pub use links::SpanLinks;
pub use provider::{Builder, TracerProvider};
pub use sampler::{Sampler, ShouldSample};
pub use span::Span;
Expand All @@ -40,14 +42,19 @@ mod runtime_tests;
#[cfg(all(test, feature = "testing"))]
mod tests {
use super::*;
use crate::testing::trace::InMemorySpanExporterBuilder;
use crate::{
testing::trace::InMemorySpanExporterBuilder, trace::span_limit::DEFAULT_MAX_LINKS_PER_SPAN,
};
use opentelemetry::{
trace::{Span, Tracer, TracerProvider as _},
trace::{
Link, Span, SpanBuilder, SpanContext, SpanId, TraceFlags, TraceId, Tracer,
TracerProvider as _,
},
KeyValue,
};

#[test]
fn tracing_in_span() {
fn in_span() {
// Arrange
let exporter = InMemorySpanExporterBuilder::new().build();
let provider = TracerProvider::builder()
Expand All @@ -71,7 +78,7 @@ mod tests {
}

#[test]
fn tracing_tracer_start() {
fn tracer_start() {
// Arrange
let exporter = InMemorySpanExporterBuilder::new().build();
let provider = TracerProvider::builder()
Expand All @@ -94,4 +101,47 @@ mod tests {
assert_eq!(span.name, "span_name");
assert_eq!(span.instrumentation_lib.name, "test_tracer");
}

#[test]
fn exceed_span_links_limit() {
// Arrange
let exporter = InMemorySpanExporterBuilder::new().build();
let provider = TracerProvider::builder()
.with_span_processor(SimpleSpanProcessor::new(Box::new(exporter.clone())))
.build();

// Act
let tracer = provider.tracer("test_tracer");

let mut links = Vec::new();
for _i in 0..(DEFAULT_MAX_LINKS_PER_SPAN * 2) {
links.push(Link::new(
SpanContext::new(
TraceId::from_u128(12),
SpanId::from_u64(12),
TraceFlags::default(),
false,
Default::default(),
),
Vec::new(),
))
}

let span_builder = SpanBuilder::from_name("span_name").with_links(links);
let mut span = tracer.build(span_builder);
span.end();
provider.force_flush();

// Assert
let exported_spans = exporter
.get_finished_spans()
.expect("Spans are expected to be exported.");
assert_eq!(exported_spans.len(), 1);
let span = &exported_spans[0];
assert_eq!(span.name, "span_name");
assert_eq!(
span.span_links.links.len(),
DEFAULT_MAX_LINKS_PER_SPAN as usize
);
}
}