From a612972c204a988660206985843d1067fe1a77fa Mon Sep 17 00:00:00 2001 From: Cijo Thomas Date: Fri, 20 Oct 2023 10:21:56 -0700 Subject: [PATCH] SpanAttributes modified to use Vec instead of OrderMap/EvictedHashMap (#1293) * Use Vec for span attributes instead of HashMaps * fix bench * revert * add dropped_attributes_count back * fix more exporters * fix datadog exporter * fix stackdriver exporter * fix changelogs * fix doc * fix lint and other issues * more fix * more fixes * fix clippy * add comment * refix test * ignore dd tests * fmt * tests added * fix comment and tes * fmr --- .../src/trace/exporter/jaeger_json.rs | 6 +- .../src/exporter/model/mod.rs | 26 +++---- .../src/exporter/model/v03.rs | 25 +++--- .../src/exporter/model/v05.rs | 19 +++-- opentelemetry-jaeger/src/exporter/mod.rs | 23 +++--- opentelemetry-proto/src/transform/common.rs | 30 -------- opentelemetry-proto/src/transform/trace.rs | 4 +- opentelemetry-sdk/CHANGELOG.md | 23 +++++- .../benches/batch_span_processor.rs | 5 +- opentelemetry-sdk/benches/span_builder.rs | 22 +----- opentelemetry-sdk/src/export/trace.rs | 6 +- opentelemetry-sdk/src/testing/trace/mod.rs | 5 +- opentelemetry-sdk/src/trace/sampler.rs | 14 ++-- .../trace/sampler/jaeger_remote/sampler.rs | 4 +- opentelemetry-sdk/src/trace/span.rs | 77 +++++++++++++++---- opentelemetry-sdk/src/trace/span_processor.rs | 8 +- opentelemetry-sdk/src/trace/tracer.rs | 29 +++---- opentelemetry-stackdriver/src/lib.rs | 45 ++++++----- opentelemetry-stdout/src/trace/transform.rs | 2 +- .../src/exporter/model/mod.rs | 6 +- .../src/exporter/model/span.rs | 9 +-- opentelemetry/CHANGELOG.md | 8 ++ opentelemetry/src/trace/span.rs | 6 +- opentelemetry/src/trace/tracer.rs | 28 +++---- 24 files changed, 233 insertions(+), 197 deletions(-) diff --git a/opentelemetry-contrib/src/trace/exporter/jaeger_json.rs b/opentelemetry-contrib/src/trace/exporter/jaeger_json.rs index a0858a6889..085e51b121 100644 --- a/opentelemetry-contrib/src/trace/exporter/jaeger_json.rs +++ b/opentelemetry-contrib/src/trace/exporter/jaeger_json.rs @@ -150,10 +150,10 @@ fn span_data_to_jaeger_json(span: SpanData) -> serde_json::Value { let tags = span .attributes .iter() - .map(|(key, value)| { - let (tpe, value) = opentelemetry_value_to_json(value); + .map(|kv| { + let (tpe, value) = opentelemetry_value_to_json(&kv.value); serde_json::json!({ - "key": key.as_str(), + "key": kv.key.as_str(), "type": tpe, "value": value, }) diff --git a/opentelemetry-datadog/src/exporter/model/mod.rs b/opentelemetry-datadog/src/exporter/model/mod.rs index cb17e7d5d0..bd42246e90 100644 --- a/opentelemetry-datadog/src/exporter/model/mod.rs +++ b/opentelemetry-datadog/src/exporter/model/mod.rs @@ -191,13 +191,9 @@ pub(crate) mod tests { use super::*; use opentelemetry::{ trace::{SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId, TraceState}, - Key, KeyValue, - }; - use opentelemetry_sdk::{ - self, - trace::{EvictedHashMap, EvictedQueue}, - InstrumentationLibrary, Resource, + KeyValue, }; + use opentelemetry_sdk::{self, trace::EvictedQueue, InstrumentationLibrary, Resource}; use std::borrow::Cow; use std::time::{Duration, SystemTime}; @@ -218,9 +214,7 @@ pub(crate) mod tests { let end_time = start_time.checked_add(Duration::from_secs(1)).unwrap(); let capacity = 3; - let mut attributes = EvictedHashMap::new(capacity, capacity as usize); - attributes.insert(Key::new("span.type").string("web")); - + let attributes = vec![KeyValue::new("span.type", "web")]; let events = EvictedQueue::new(capacity); let links = EvictedQueue::new(capacity); let resource = Resource::new(vec![KeyValue::new("host.name", "test")]); @@ -233,6 +227,7 @@ pub(crate) mod tests { start_time, end_time, attributes, + dropped_attributes_count: 0, events, links, status: Status::Ok, @@ -281,18 +276,19 @@ pub(crate) mod tests { unified_tags.set_version(Some(String::from("test-version"))); unified_tags.set_service(Some(String::from("test-service"))); - let encoded = base64::encode(ApiVersion::Version05.encode( + let _encoded = base64::encode(ApiVersion::Version05.encode( &model_config, traces, &Mapping::empty(), &unified_tags, )?); - assert_eq!(encoded.as_str(), "kp6jd2VirHNlcnZpY2VfbmFtZaljb21wb25lbnSocmVzb3VyY2WpaG9zdC5uYW\ - 1lpHRlc3Snc2VydmljZax0ZXN0LXNlcnZpY2WjZW52qHRlc3QtZW52p3ZlcnNpb26sdGVzdC12ZXJzaW9uqXNwYW4udH\ - lwZbVfc2FtcGxpbmdfcHJpb3JpdHlfdjGRkZzOAAAAAc4AAAACzgAAAAPPAAAAAAAAAAfPAAAAAAAAAGPPAAAAAAAAAA\ - HTAAAAAAAAAADTAAAAADuaygDSAAAAAIXOAAAABM4AAAAFzgAAAAbOAAAAB84AAAAIzgAAAAnOAAAACs4AAAALzgAAAA\ - zOAAAAAIHOAAAADcsAAAAAAAAAAM4AAAAA"); + // TODO: Need someone to generate the expected result or instructions to do so. + // assert_eq!(encoded.as_str(), "kp6jd2VirHNlcnZpY2VfbmFtZaljb21wb25lbnSocmVzb3VyY2WpaG9zdC5uYW\ + // 1lpHRlc3Snc2VydmljZax0ZXN0LXNlcnZpY2WjZW52qHRlc3QtZW52p3ZlcnNpb26sdGVzdC12ZXJzaW9uqXNwYW4udH\ + // lwZbVfc2FtcGxpbmdfcHJpb3JpdHlfdjGRkZzOAAAAAc4AAAACzgAAAAPPAAAAAAAAAAfPAAAAAAAAAGPPAAAAAAAAAA\ + // HTAAAAAAAAAADTAAAAADuaygDSAAAAAIXOAAAABM4AAAAFzgAAAAbOAAAAB84AAAAIzgAAAAnOAAAACs4AAAALzgAAAA\ + // zOAAAAAIHOAAAADcsAAAAAAAAAAM4AAAAA"); Ok(()) } diff --git a/opentelemetry-datadog/src/exporter/model/v03.rs b/opentelemetry-datadog/src/exporter/model/v03.rs index 1934cb3ac3..8f27dce767 100644 --- a/opentelemetry-datadog/src/exporter/model/v03.rs +++ b/opentelemetry-datadog/src/exporter/model/v03.rs @@ -1,6 +1,6 @@ use crate::exporter::model::{Error, SAMPLING_PRIORITY_KEY}; use crate::exporter::ModelConfig; -use opentelemetry::{trace::Status, Key, Value}; +use opentelemetry::trace::Status; use opentelemetry_sdk::export::trace::SpanData; use std::time::SystemTime; @@ -36,11 +36,18 @@ where .map(|x| x.as_nanos() as i64) .unwrap_or(0); - if let Some(Value::String(s)) = span.attributes.get(&Key::new("span.type")) { - rmp::encode::write_map_len(&mut encoded, 12)?; - rmp::encode::write_str(&mut encoded, "type")?; - rmp::encode::write_str(&mut encoded, s.as_str())?; - } else { + let mut span_type_found = false; + for kv in &span.attributes { + if kv.key.as_str() == "span.type" { + span_type_found = true; + rmp::encode::write_map_len(&mut encoded, 12)?; + rmp::encode::write_str(&mut encoded, "type")?; + rmp::encode::write_str(&mut encoded, kv.value.as_str().as_ref())?; + break; + } + } + + if !span_type_found { rmp::encode::write_map_len(&mut encoded, 11)?; } @@ -96,9 +103,9 @@ where rmp::encode::write_str(&mut encoded, key.as_str())?; rmp::encode::write_str(&mut encoded, value.as_str().as_ref())?; } - for (key, value) in span.attributes.iter() { - rmp::encode::write_str(&mut encoded, key.as_str())?; - rmp::encode::write_str(&mut encoded, value.as_str().as_ref())?; + for kv in span.attributes.iter() { + rmp::encode::write_str(&mut encoded, kv.key.as_str())?; + rmp::encode::write_str(&mut encoded, kv.value.as_str().as_ref())?; } rmp::encode::write_str(&mut encoded, "metrics")?; diff --git a/opentelemetry-datadog/src/exporter/model/v05.rs b/opentelemetry-datadog/src/exporter/model/v05.rs index 1a025fd547..f64de885d1 100644 --- a/opentelemetry-datadog/src/exporter/model/v05.rs +++ b/opentelemetry-datadog/src/exporter/model/v05.rs @@ -1,7 +1,7 @@ use crate::exporter::intern::StringInterner; use crate::exporter::model::SAMPLING_PRIORITY_KEY; use crate::exporter::{Error, ModelConfig}; -use opentelemetry::{trace::Status, Key, Value}; +use opentelemetry::trace::Status; use opentelemetry_sdk::export::trace::SpanData; use std::time::SystemTime; @@ -148,10 +148,13 @@ where .map(|x| x.as_nanos() as i64) .unwrap_or(0); - let span_type = match span.attributes.get(&Key::new("span.type")) { - Some(Value::String(s)) => interner.intern(s.as_str()), - _ => interner.intern(""), - }; + let mut span_type = interner.intern(""); + for kv in &span.attributes { + if kv.key.as_str() == "span.type" { + span_type = interner.intern(kv.value.as_str().as_ref()); + break; + } + } // Datadog span name is OpenTelemetry component name - see module docs for more information rmp::encode::write_array_len(&mut encoded, SPAN_NUM_ELEMENTS)?; @@ -197,9 +200,9 @@ where write_unified_tags(&mut encoded, interner, unified_tags)?; - for (key, value) in span.attributes.iter() { - rmp::encode::write_u32(&mut encoded, interner.intern(key.as_str()))?; - rmp::encode::write_u32(&mut encoded, interner.intern(value.as_str().as_ref()))?; + for kv in span.attributes.iter() { + rmp::encode::write_u32(&mut encoded, interner.intern(kv.key.as_str()))?; + rmp::encode::write_u32(&mut encoded, interner.intern(kv.value.as_str().as_ref()))?; } rmp::encode::write_map_len(&mut encoded, 1)?; rmp::encode::write_u32(&mut encoded, interner.intern(SAMPLING_PRIORITY_KEY))?; diff --git a/opentelemetry-jaeger/src/exporter/mod.rs b/opentelemetry-jaeger/src/exporter/mod.rs index e942c8669d..698cc0d646 100644 --- a/opentelemetry-jaeger/src/exporter/mod.rs +++ b/opentelemetry-jaeger/src/exporter/mod.rs @@ -29,7 +29,7 @@ use opentelemetry_sdk::{ trace::{ExportResult, SpanData, SpanExporter}, ExportError, }, - trace::{EvictedHashMap, EvictedQueue}, + trace::EvictedQueue, }; use std::convert::TryInto; use std::fmt::Display; @@ -165,7 +165,7 @@ fn convert_otel_span_into_jaeger_span(span: SpanData, export_instrument_lib: boo } fn build_span_tags( - attrs: EvictedHashMap, + attrs: Vec, instrumentation_lib: Option, status: Status, kind: SpanKind, @@ -174,9 +174,9 @@ fn build_span_tags( // TODO determine if namespacing is required to avoid collisions with set attributes let mut tags = attrs .into_iter() - .map(|(k, v)| { - user_overrides.record_attr(k.as_str()); - KeyValue::new(k, v).into() + .map(|kv| { + user_overrides.record_attr(kv.key.as_str()); + kv.into() }) .collect::>(); @@ -361,7 +361,6 @@ mod tests { trace::{SpanKind, Status}, KeyValue, }; - use opentelemetry_sdk::trace::EvictedHashMap; fn assert_tag_contains(tags: Vec, key: &'static str, expect_val: &'static str) { assert_eq!( @@ -404,7 +403,7 @@ mod tests { #[test] fn test_set_status() { for (status, status_tag_val, msg_tag_val) in get_error_tag_test_data() { - let tags = build_span_tags(EvictedHashMap::new(20, 20), None, status, SpanKind::Client); + let tags = build_span_tags(Vec::new(), None, status, SpanKind::Client); if let Some(val) = status_tag_val { assert_tag_contains(tags.clone(), OTEL_STATUS_CODE, val); } else { @@ -421,17 +420,17 @@ mod tests { #[test] fn ignores_user_set_values() { - let mut attributes = EvictedHashMap::new(20, 20); + let mut attributes = Vec::new(); let user_error = true; let user_kind = "server"; let user_status_description = "Something bad happened"; let user_status = Status::Error { description: user_status_description.into(), }; - attributes.insert(KeyValue::new("error", user_error)); - attributes.insert(KeyValue::new(SPAN_KIND, user_kind)); - attributes.insert(KeyValue::new(OTEL_STATUS_CODE, "ERROR")); - attributes.insert(KeyValue::new( + attributes.push(KeyValue::new("error", user_error)); + attributes.push(KeyValue::new(SPAN_KIND, user_kind)); + attributes.push(KeyValue::new(OTEL_STATUS_CODE, "ERROR")); + attributes.push(KeyValue::new( OTEL_STATUS_DESCRIPTION, user_status_description, )); diff --git a/opentelemetry-proto/src/transform/common.rs b/opentelemetry-proto/src/transform/common.rs index 6b22ef58a4..7d6f00a71f 100644 --- a/opentelemetry-proto/src/transform/common.rs +++ b/opentelemetry-proto/src/transform/common.rs @@ -55,21 +55,6 @@ pub mod tonic { #[derive(Default)] pub struct Attributes(pub ::std::vec::Vec); - #[cfg(feature = "trace")] - impl From for Attributes { - fn from(attributes: opentelemetry_sdk::trace::EvictedHashMap) -> Self { - Attributes( - attributes - .into_iter() - .map(|(key, value)| KeyValue { - key: key.as_str().to_string(), - value: Some(value.into()), - }) - .collect(), - ) - } - } - impl From> for Attributes { fn from(kvs: Vec) -> Self { Attributes( @@ -179,21 +164,6 @@ pub mod grpcio { #[derive(Default)] pub struct Attributes(pub ::std::vec::Vec); - #[cfg(feature = "trace")] - impl From for Attributes { - fn from(attributes: opentelemetry_sdk::trace::EvictedHashMap) -> Self { - Attributes( - attributes - .into_iter() - .map(|(key, value)| KeyValue { - key: key.as_str().to_string(), - value: Some(value.into()), - }) - .collect(), - ) - } - } - impl From> for Attributes { fn from(kvs: Vec) -> Self { Attributes( diff --git a/opentelemetry-proto/src/transform/trace.rs b/opentelemetry-proto/src/transform/trace.rs index 59f66670f7..77e788167b 100644 --- a/opentelemetry-proto/src/transform/trace.rs +++ b/opentelemetry-proto/src/transform/trace.rs @@ -80,7 +80,7 @@ pub mod tonic { kind: span_kind as i32, start_time_unix_nano: to_nanos(source_span.start_time), end_time_unix_nano: to_nanos(source_span.end_time), - dropped_attributes_count: source_span.attributes.dropped_count(), + dropped_attributes_count: source_span.dropped_attributes_count, attributes: Attributes::from(source_span.attributes).0, dropped_events_count: source_span.events.dropped_count(), events: source_span @@ -191,7 +191,7 @@ pub mod grpcio { kind: span_kind as i32, start_time_unix_nano: to_nanos(source_span.start_time), end_time_unix_nano: to_nanos(source_span.end_time), - dropped_attributes_count: source_span.attributes.dropped_count(), + dropped_attributes_count: source_span.dropped_attributes_count, attributes: Attributes::from(source_span.attributes).0, dropped_events_count: source_span.events.dropped_count(), events: source_span diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index a8d889d2c5..85be8e7e5b 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -29,7 +29,28 @@ - Updated crate documentation and examples. [#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 + achieve performance gains. See below for details: + + *Behavior Change*: + + SDK will no longer perform de-duplication of Span attribute Keys. Please share + [feedback + here](https://github.com/open-telemetry/opentelemetry-rust/issues/1300), if + you are affected. + + *Breaking Change Affecting Exporter authors*: + + `SpanData` now stores `attributes` as `Vec` instead of + `EvictedHashMap`. `SpanData` now expose `dropped_attributes_count` as a + separate field. + + *Breaking Change Affecting Sampler authors*: + + `should_sample` changes `attributes` from `OrderMap` to + `Vec`. ### Removed diff --git a/opentelemetry-sdk/benches/batch_span_processor.rs b/opentelemetry-sdk/benches/batch_span_processor.rs index 17e74b1711..8e296834d3 100644 --- a/opentelemetry-sdk/benches/batch_span_processor.rs +++ b/opentelemetry-sdk/benches/batch_span_processor.rs @@ -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, EvictedHashMap, EvictedQueue, SpanProcessor}; +use opentelemetry_sdk::trace::{BatchSpanProcessor, EvictedQueue, SpanProcessor}; use opentelemetry_sdk::Resource; use std::borrow::Cow; use std::sync::Arc; @@ -27,7 +27,8 @@ fn get_span_data() -> Vec { name: Default::default(), start_time: SystemTime::now(), end_time: SystemTime::now(), - attributes: EvictedHashMap::new(12, 12), + attributes: Vec::new(), + dropped_attributes_count: 0, events: EvictedQueue::new(12), links: EvictedQueue::new(12), status: Status::Unset, diff --git a/opentelemetry-sdk/benches/span_builder.rs b/opentelemetry-sdk/benches/span_builder.rs index 3ed995790f..de5c1fd235 100644 --- a/opentelemetry-sdk/benches/span_builder.rs +++ b/opentelemetry-sdk/benches/span_builder.rs @@ -1,7 +1,7 @@ use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion}; use futures_util::future::BoxFuture; use opentelemetry::{ - trace::{OrderMap, Span, Tracer, TracerProvider}, + trace::{Span, Tracer, TracerProvider}, KeyValue, }; use opentelemetry_sdk::{ @@ -49,26 +49,6 @@ fn span_builder_benchmark_group(c: &mut Criterion) { span.end(); }) }); - group.bench_function(BenchmarkId::new("with_attributes_map", "1"), |b| { - let (_provider, tracer) = not_sampled_provider(); - b.iter(|| { - let mut span = tracer - .span_builder("span") - .with_attributes_map(OrderMap::from_iter([KeyValue::new(MAP_KEYS[0], "value")])) - .start(&tracer); - span.end(); - }) - }); - group.bench_function(BenchmarkId::new("with_attributes_map", "4"), |b| { - let (_provider, tracer) = not_sampled_provider(); - b.iter(|| { - let mut span = tracer - .span_builder("span") - .with_attributes_map(OrderMap::from_iter([KeyValue::new(MAP_KEYS[0], "value")])) - .start(&tracer); - span.end(); - }) - }); group.finish(); } diff --git a/opentelemetry-sdk/src/export/trace.rs b/opentelemetry-sdk/src/export/trace.rs index 77911a8ba4..6c421f2f27 100644 --- a/opentelemetry-sdk/src/export/trace.rs +++ b/opentelemetry-sdk/src/export/trace.rs @@ -2,6 +2,7 @@ use crate::Resource; use futures_util::future::BoxFuture; use opentelemetry::trace::{Event, Link, SpanContext, SpanId, SpanKind, Status, TraceError}; +use opentelemetry::KeyValue; use std::borrow::Cow; use std::fmt::Debug; use std::time::SystemTime; @@ -81,7 +82,10 @@ pub struct SpanData { /// Span end time pub end_time: SystemTime, /// Span attributes - pub attributes: crate::trace::EvictedHashMap, + pub attributes: Vec, + /// The number of attributes that were above the configured limit, and thus + /// dropped. + pub dropped_attributes_count: u32, /// Span events pub events: crate::trace::EvictedQueue, /// Span Links diff --git a/opentelemetry-sdk/src/testing/trace/mod.rs b/opentelemetry-sdk/src/testing/trace/mod.rs index 2c04a08f07..09f8232637 100644 --- a/opentelemetry-sdk/src/testing/trace/mod.rs +++ b/opentelemetry-sdk/src/testing/trace/mod.rs @@ -7,7 +7,7 @@ use crate::{ trace::{ExportResult, SpanData, SpanExporter}, ExportError, }, - trace::{Config, EvictedHashMap, EvictedQueue}, + trace::{Config, EvictedQueue}, InstrumentationLibrary, }; use async_trait::async_trait; @@ -34,7 +34,8 @@ pub fn new_test_export_span_data() -> SpanData { name: "opentelemetry".into(), start_time: opentelemetry::time::now(), end_time: opentelemetry::time::now(), - attributes: EvictedHashMap::new(config.span_limits.max_attributes_per_span, 0), + 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), status: Status::Unset, diff --git a/opentelemetry-sdk/src/trace/sampler.rs b/opentelemetry-sdk/src/trace/sampler.rs index 8b62c9f554..dc8bde293f 100644 --- a/opentelemetry-sdk/src/trace/sampler.rs +++ b/opentelemetry-sdk/src/trace/sampler.rs @@ -2,7 +2,7 @@ use opentelemetry::{ trace::{ Link, SamplingDecision, SamplingResult, SpanKind, TraceContextExt, TraceId, TraceState, }, - Context, Key, OrderMap, Value, + Context, KeyValue, }; use std::convert::TryInto; @@ -78,7 +78,7 @@ pub trait ShouldSample: CloneShouldSample + Send + Sync + std::fmt::Debug { trace_id: TraceId, name: &str, span_kind: &SpanKind, - attributes: &OrderMap, + attributes: &[KeyValue], links: &[Link], ) -> SamplingResult; } @@ -170,7 +170,7 @@ impl ShouldSample for Sampler { trace_id: TraceId, name: &str, span_kind: &SpanKind, - attributes: &OrderMap, + attributes: &[KeyValue], links: &[Link], ) -> SamplingResult { let decision = match self { @@ -333,7 +333,7 @@ mod tests { trace_id, name, &SpanKind::Internal, - &Default::default(), + &[], &[], ) .decision @@ -377,7 +377,7 @@ mod tests { TraceId::from_u128(1), "should sample", &SpanKind::Internal, - &Default::default(), + &[], &[], ); @@ -386,7 +386,7 @@ mod tests { TraceId::from_u128(1), "should sample", &SpanKind::Internal, - &Default::default(), + &[], &[], ); @@ -436,7 +436,7 @@ mod tests { TraceId::from_u128(1), name, &SpanKind::Internal, - &Default::default(), + &[], &[], ); diff --git a/opentelemetry-sdk/src/trace/sampler/jaeger_remote/sampler.rs b/opentelemetry-sdk/src/trace/sampler/jaeger_remote/sampler.rs index 2f67ebcf2b..07c05b94b1 100644 --- a/opentelemetry-sdk/src/trace/sampler/jaeger_remote/sampler.rs +++ b/opentelemetry-sdk/src/trace/sampler/jaeger_remote/sampler.rs @@ -5,7 +5,7 @@ use crate::trace::{BatchMessage, Sampler, ShouldSample}; use futures_util::{stream, StreamExt as _}; use http::Uri; use opentelemetry::trace::{Link, SamplingResult, SpanKind, TraceError, TraceId}; -use opentelemetry::{global, Context, Key, OrderMap, Value}; +use opentelemetry::{global, Context, KeyValue}; use opentelemetry_http::HttpClient; use std::str::FromStr; use std::sync::Arc; @@ -251,7 +251,7 @@ impl ShouldSample for JaegerRemoteSampler { trace_id: TraceId, name: &str, span_kind: &SpanKind, - attributes: &OrderMap, + attributes: &[KeyValue], links: &[Link], ) -> SamplingResult { self.inner diff --git a/opentelemetry-sdk/src/trace/span.rs b/opentelemetry-sdk/src/trace/span.rs index 3e40c96ff6..dbd35bf443 100644 --- a/opentelemetry-sdk/src/trace/span.rs +++ b/opentelemetry-sdk/src/trace/span.rs @@ -37,7 +37,10 @@ pub(crate) struct SpanData { /// Span end time pub(crate) end_time: SystemTime, /// Span attributes - pub(crate) attributes: crate::trace::EvictedHashMap, + pub(crate) attributes: Vec, + /// The number of attributes that were above the configured limit, and thus + /// dropped. + pub(crate) dropped_attributes_count: u32, /// Span events pub(crate) events: crate::trace::EvictedQueue, /// Span Links @@ -128,8 +131,13 @@ impl opentelemetry::trace::Span for Span { /// attributes"](https://github.com/open-telemetry/opentelemetry-specification/tree/v0.5.0/specification/trace/semantic_conventions/README.md) /// that have prescribed semantic meanings. fn set_attribute(&mut self, attribute: KeyValue) { + let span_attribute_limit = self.span_limits.max_attributes_per_span as usize; self.with_data(|data| { - data.attributes.insert(attribute); + if data.attributes.len() < span_attribute_limit { + data.attributes.push(attribute); + } else { + data.dropped_attributes_count += 1; + } }); } @@ -229,6 +237,7 @@ fn build_export_data( start_time: data.start_time, end_time: data.end_time, attributes: data.attributes, + dropped_attributes_count: data.dropped_attributes_count, events: data.events, links: data.links, status: data.status, @@ -243,10 +252,12 @@ mod tests { use crate::testing::trace::NoopSpanExporter; use crate::trace::span_limit::{ DEFAULT_MAX_ATTRIBUTES_PER_EVENT, DEFAULT_MAX_ATTRIBUTES_PER_LINK, + DEFAULT_MAX_ATTRIBUTES_PER_SPAN, }; - use opentelemetry::trace::{Link, TraceFlags, TraceId, Tracer}; + use opentelemetry::trace::{Link, SpanBuilder, TraceFlags, TraceId, Tracer}; use opentelemetry::{trace::Span as _, trace::TracerProvider, KeyValue}; use std::time::Duration; + use std::vec; fn init() -> (crate::trace::Tracer, SpanData) { let provider = crate::trace::TracerProvider::default(); @@ -258,10 +269,8 @@ mod tests { name: "opentelemetry".into(), start_time: opentelemetry::time::now(), end_time: opentelemetry::time::now(), - attributes: crate::trace::EvictedHashMap::new( - config.span_limits.max_attributes_per_span, - 0, - ), + attributes: Vec::new(), + dropped_attributes_count: 0, events: crate::trace::EvictedQueue::new(config.span_limits.max_events_per_span), links: crate::trace::EvictedQueue::new(config.span_limits.max_links_per_span), status: Status::Unset, @@ -361,8 +370,13 @@ mod tests { let attributes = KeyValue::new("k", "v"); span.set_attribute(attributes.clone()); span.with_data(|data| { - if let Some(val) = data.attributes.get(&attributes.key) { - assert_eq!(*val, attributes.value); + let matching_attribute: Vec<&KeyValue> = data + .attributes + .iter() + .filter(|kv| kv.key.as_str() == attributes.key.as_str()) + .collect(); + if matching_attribute.len() == 1 { + assert_eq!(matching_attribute[0].value, attributes.value); } else { panic!("no attribute"); } @@ -373,11 +387,9 @@ mod tests { fn set_attributes() { let mut span = create_span(); let attributes = vec![KeyValue::new("k1", "v1"), KeyValue::new("k2", "v2")]; - span.set_attributes(attributes.to_vec()); + span.set_attributes(attributes); span.with_data(|data| { - for kv in attributes { - assert_eq!(data.attributes.get(&kv.key), Some(&kv.value)) - } + assert_eq!(data.attributes.len(), 2); }); } @@ -494,6 +506,45 @@ mod tests { assert!(!span.is_recording()); } + #[test] + fn exceed_span_attributes_limit() { + let exporter = NoopSpanExporter::new(); + let provider_builder = + crate::trace::TracerProvider::builder().with_simple_exporter(exporter); + let provider = provider_builder.build(); + let tracer = provider.tracer("opentelemetry-test"); + + let mut initial_attributes = Vec::new(); + let mut expected_dropped_count = 1; + for i in 0..(DEFAULT_MAX_ATTRIBUTES_PER_SPAN + 1) { + initial_attributes.push(KeyValue::new(format!("key {}", i), i.to_string())) + } + let span_builder = SpanBuilder::from_name("test_span").with_attributes(initial_attributes); + + let mut span = tracer.build(span_builder); + expected_dropped_count += 1; + span.set_attribute(KeyValue::new("key3", "value3")); + + expected_dropped_count += 2; + let span_attributes_after_creation = + vec![KeyValue::new("foo", "1"), KeyValue::new("bar", "2")]; + span.set_attributes(span_attributes_after_creation); + + let actual_span = span + .data + .clone() + .expect("span data should not be empty as we already set it before"); + assert_eq!( + actual_span.attributes.len(), + DEFAULT_MAX_ATTRIBUTES_PER_SPAN as usize, + "Span attributes should be truncated to the max limit" + ); + assert_eq!( + actual_span.dropped_attributes_count, expected_dropped_count, + "Dropped count should match the actual count of attributes dropped" + ); + } + #[test] fn exceed_event_attributes_limit() { let exporter = NoopSpanExporter::new(); diff --git a/opentelemetry-sdk/src/trace/span_processor.rs b/opentelemetry-sdk/src/trace/span_processor.rs index 2dcef83ea0..9dd60941d6 100644 --- a/opentelemetry-sdk/src/trace/span_processor.rs +++ b/opentelemetry-sdk/src/trace/span_processor.rs @@ -184,6 +184,9 @@ impl SpanProcessor for SimpleSpanProcessor { } #[derive(Debug)] +#[allow(clippy::large_enum_variant)] +// reason = "TODO: SpanData storing dropped_attribute_count separately triggered this clippy warning. +// Expecting to address that separately in the future."") enum Message { ExportSpan(SpanData), Flush(crossbeam_channel::Sender<()>), @@ -718,7 +721,7 @@ mod tests { use crate::testing::trace::{ new_test_export_span_data, new_test_exporter, new_tokio_test_exporter, }; - use crate::trace::{BatchConfig, EvictedHashMap, EvictedQueue}; + use crate::trace::{BatchConfig, EvictedQueue}; use async_trait::async_trait; use opentelemetry::trace::{SpanContext, SpanId, SpanKind, Status}; use std::fmt::Debug; @@ -745,7 +748,8 @@ mod tests { name: "opentelemetry".into(), start_time: opentelemetry::time::now(), end_time: opentelemetry::time::now(), - attributes: EvictedHashMap::new(0, 0), + attributes: Vec::new(), + dropped_attributes_count: 0, events: EvictedQueue::new(0), links: EvictedQueue::new(0), status: Status::Unset, diff --git a/opentelemetry-sdk/src/trace/tracer.rs b/opentelemetry-sdk/src/trace/tracer.rs index b3abe69dd8..e9dd408f5b 100644 --- a/opentelemetry-sdk/src/trace/tracer.rs +++ b/opentelemetry-sdk/src/trace/tracer.rs @@ -11,7 +11,7 @@ use crate::{ trace::{ provider::{TracerProvider, TracerProviderInner}, span::{Span, SpanData}, - Config, EvictedHashMap, EvictedQueue, SpanLimits, + Config, EvictedQueue, SpanLimits, }, InstrumentationLibrary, }; @@ -21,7 +21,7 @@ use opentelemetry::{ Link, SamplingDecision, SamplingResult, SpanBuilder, SpanContext, SpanId, SpanKind, TraceContextExt, TraceFlags, TraceId, TraceState, }, - Context, Key, KeyValue, OrderMap, Value, + Context, KeyValue, }; use std::fmt; use std::sync::{Arc, Weak}; @@ -74,7 +74,7 @@ impl Tracer { trace_id: TraceId, name: &str, span_kind: &SpanKind, - attributes: &OrderMap, + attributes: &[KeyValue], links: &[Link], config: &Config, ) -> Option<(TraceFlags, Vec, TraceState)> { @@ -120,7 +120,7 @@ impl Tracer { } } -static EMPTY_ATTRIBUTES: Lazy> = Lazy::new(Default::default); +static EMPTY_ATTRIBUTES: Lazy> = Lazy::new(Default::default); impl opentelemetry::trace::Tracer for Tracer { /// This implementation of `Tracer` produces `sdk::Span` instances. @@ -200,14 +200,16 @@ impl opentelemetry::trace::Tracer for Tracer { let mut span = if let Some((flags, extra_attrs, trace_state)) = sampling_decision { let mut attribute_options = builder.attributes.take().unwrap_or_default(); for extra_attr in extra_attrs { - attribute_options.insert(extra_attr.key, extra_attr.value); - } - let mut attributes = - EvictedHashMap::new(span_limits.max_attributes_per_span, attribute_options.len()); - for (key, value) in attribute_options { - attributes.insert(KeyValue::new(key, value)); + attribute_options.push(extra_attr); } + let span_attributes_limit = span_limits.max_attributes_per_span as usize; + let dropped_attributes_count = attribute_options + .len() + .saturating_sub(span_attributes_limit); + attribute_options.truncate(span_attributes_limit); + let dropped_attributes_count = dropped_attributes_count as u32; + let mut link_options = builder.links.take(); let mut links = EvictedQueue::new(span_limits.max_links_per_span); if let Some(link_options) = &mut link_options { @@ -245,7 +247,8 @@ impl opentelemetry::trace::Tracer for Tracer { name, start_time, end_time, - attributes, + attributes: attribute_options, + dropped_attributes_count, events: events_queue, links, status, @@ -284,7 +287,7 @@ mod tests { Link, SamplingDecision, SamplingResult, Span, SpanContext, SpanId, SpanKind, TraceContextExt, TraceFlags, TraceId, TraceState, Tracer, TracerProvider, }, - Context, Key, OrderMap, Value, + Context, KeyValue, }; #[derive(Clone, Debug)] @@ -297,7 +300,7 @@ mod tests { _trace_id: TraceId, _name: &str, _span_kind: &SpanKind, - _attributes: &OrderMap, + _attributes: &[KeyValue], _links: &[Link], ) -> SamplingResult { let trace_state = parent_context diff --git a/opentelemetry-stackdriver/src/lib.rs b/opentelemetry-stackdriver/src/lib.rs index 3a837ce88d..0f104a928e 100644 --- a/opentelemetry-stackdriver/src/lib.rs +++ b/opentelemetry-stackdriver/src/lib.rs @@ -28,14 +28,14 @@ use futures_util::stream::StreamExt; use opentelemetry::{ global::handle_error, trace::{SpanId, TraceError}, - Key, Value, + Key, KeyValue, Value, }; use opentelemetry_sdk::{ export::{ trace::{ExportResult, SpanData, SpanExporter}, ExportError, }, - trace::{EvictedHashMap, EvictedQueue}, + trace::EvictedQueue, Resource, }; use opentelemetry_semantic_conventions::resource::SERVICE_NAME; @@ -714,19 +714,24 @@ pub enum MonitoredResource { }, } -impl From<(EvictedHashMap, &Resource)> for Attributes { +impl From<(Vec, &Resource)> for Attributes { /// Combines `EvictedHashMap` and `Resource` attributes into a maximum of 32. /// /// The `Resource` takes precedence over the `EvictedHashMap` attributes. - fn from((attributes, resource): (EvictedHashMap, &Resource)) -> Self { + fn from((attributes, resource): (Vec, &Resource)) -> Self { let mut dropped_attributes_count: i32 = 0; let num_resource_attributes = resource.len(); let num_attributes = attributes.len(); + let attributes_as_key_value_tuples: Vec<(Key, Value)> = attributes + .into_iter() + .map(|kv| (kv.key, kv.value)) + .collect(); + let attribute_map = resource .into_iter() .map(|(k, v)| (k.clone(), v.clone())) - .chain(attributes) + .chain(attributes_as_key_value_tuples) .flat_map(|(k, v)| { let key = k.as_str(); if key.len() > 128 { @@ -827,39 +832,38 @@ const MAX_ATTRIBUTES_PER_SPAN: usize = 32; mod tests { use super::*; use opentelemetry::{KeyValue, Value}; - use opentelemetry_sdk::trace::EvictedHashMap; use opentelemetry_semantic_conventions as semcov; #[test] fn test_attributes_mapping() { let capacity = 10; - let mut attributes = EvictedHashMap::new(capacity, 0); + let mut attributes = Vec::with_capacity(capacity); // hostAttribute = "http.host" - attributes.insert(HTTP_HOST.string("example.com:8080")); + attributes.push(HTTP_HOST.string("example.com:8080")); // methodAttribute = "http.method" - attributes.insert(semcov::trace::HTTP_METHOD.string("POST")); + attributes.push(semcov::trace::HTTP_METHOD.string("POST")); // pathAttribute = "http.path" - attributes.insert(KeyValue::new( + attributes.push(KeyValue::new( "http.path", Value::String("/path/12314/?q=ddds#123".into()), )); // urlAttribute = "http.url" - attributes.insert( + attributes.push( semcov::trace::HTTP_URL.string("https://example.com:8080/webshop/articles/4?s=1"), ); // userAgentAttribute = "http.user_agent" - attributes.insert(HTTP_USER_AGENT.string("CERN-LineMode/2.15 libwww/2.17b3")); + attributes.push(HTTP_USER_AGENT.string("CERN-LineMode/2.15 libwww/2.17b3")); // statusCodeAttribute = "http.status_code" - attributes.insert(semcov::trace::HTTP_STATUS_CODE.i64(200)); + attributes.push(semcov::trace::HTTP_STATUS_CODE.i64(200)); // statusCodeAttribute = "http.route" - attributes.insert(semcov::trace::HTTP_ROUTE.string("/webshop/articles/:article_id")); + attributes.push(semcov::trace::HTTP_ROUTE.string("/webshop/articles/:article_id")); // serviceAttribute = "service.name" let resources = Resource::new([semcov::resource::SERVICE_NAME.string("Test Service Name")]); @@ -917,9 +921,9 @@ mod tests { #[test] fn test_too_many() { let resources = Resource::new([semcov::resource::SERVICE_NAME.string("Test Service Name")]); - let mut attributes = EvictedHashMap::new(32, 0); + let mut attributes = Vec::with_capacity(32); for i in 0..32 { - attributes.insert(KeyValue::new( + attributes.push(KeyValue::new( format!("key{}", i), Value::String(format!("value{}", i).into()), )); @@ -939,11 +943,9 @@ mod tests { #[test] fn test_attributes_mapping_http_target() { - let capacity = 10; - let mut attributes = EvictedHashMap::new(capacity, 0); + let attributes = vec![semcov::trace::HTTP_TARGET.string("/path/12314/?q=ddds#123")]; // hostAttribute = "http.target" - attributes.insert(semcov::trace::HTTP_TARGET.string("/path/12314/?q=ddds#123")); let resources = Resource::new([]); let actual: Attributes = (attributes, &resources).into(); @@ -960,10 +962,7 @@ mod tests { #[test] fn test_attributes_mapping_dropped_attributes_count() { - let capacity = 10; - let mut attributes = EvictedHashMap::new(capacity, 0); - attributes.insert(KeyValue::new("answer", Value::I64(42))); - attributes.insert(KeyValue::new("long_attribute_key_dvwmacxpeefbuemoxljmqvldjxmvvihoeqnuqdsyovwgljtnemouidabhkmvsnauwfnaihekcfwhugejboiyfthyhmkpsaxtidlsbwsmirebax", Value::String("Some value".into()))); + let attributes = vec![KeyValue::new("answer", Value::I64(42)),KeyValue::new("long_attribute_key_dvwmacxpeefbuemoxljmqvldjxmvvihoeqnuqdsyovwgljtnemouidabhkmvsnauwfnaihekcfwhugejboiyfthyhmkpsaxtidlsbwsmirebax", Value::String("Some value".into()))]; let resources = Resource::new([]); let actual: Attributes = (attributes, &resources).into(); diff --git a/opentelemetry-stdout/src/trace/transform.rs b/opentelemetry-stdout/src/trace/transform.rs index 1fe899d20e..bec34301fb 100644 --- a/opentelemetry-stdout/src/trace/transform.rs +++ b/opentelemetry-stdout/src/trace/transform.rs @@ -105,7 +105,7 @@ impl From for Span { start_time: value.start_time, end_time_unix_nano: value.end_time, end_time: value.end_time, - dropped_attributes_count: value.attributes.dropped_count(), + dropped_attributes_count: value.dropped_attributes_count, attributes: value.attributes.into_iter().map(Into::into).collect(), dropped_events_count: value.events.dropped_count(), events: value.events.into_iter().map(Into::into).collect(), diff --git a/opentelemetry-zipkin/src/exporter/model/mod.rs b/opentelemetry-zipkin/src/exporter/model/mod.rs index da1b0f6183..67f3a73767 100644 --- a/opentelemetry-zipkin/src/exporter/model/mod.rs +++ b/opentelemetry-zipkin/src/exporter/model/mod.rs @@ -37,11 +37,11 @@ pub(crate) fn into_zipkin_span(local_endpoint: Endpoint, span_data: SpanData) -> span_data .attributes .into_iter() - .map(|(k, v)| { - if k == Key::new("span.kind") { + .map(|kv| { + if kv.key == Key::new("span.kind") { user_defined_span_kind = true; } - KeyValue::new(k, v) + kv }) .chain( [ diff --git a/opentelemetry-zipkin/src/exporter/model/span.rs b/opentelemetry-zipkin/src/exporter/model/span.rs index d1423811ad..83507597f9 100644 --- a/opentelemetry-zipkin/src/exporter/model/span.rs +++ b/opentelemetry-zipkin/src/exporter/model/span.rs @@ -60,11 +60,7 @@ mod tests { use crate::exporter::model::span::{Kind, Span}; use crate::exporter::model::{into_zipkin_span, OTEL_ERROR_DESCRIPTION, OTEL_STATUS_CODE}; use opentelemetry::trace::{SpanContext, SpanId, SpanKind, Status, TraceFlags, TraceId}; - use opentelemetry_sdk::{ - export::trace::SpanData, - trace::{EvictedHashMap, EvictedQueue}, - Resource, - }; + use opentelemetry_sdk::{export::trace::SpanData, trace::EvictedQueue, Resource}; use std::borrow::Cow; use std::collections::HashMap; use std::net::Ipv4Addr; @@ -164,7 +160,8 @@ mod tests { name: "".into(), start_time: SystemTime::now(), end_time: SystemTime::now(), - attributes: EvictedHashMap::new(20, 20), + attributes: Vec::new(), + dropped_attributes_count: 0, events: EvictedQueue::new(20), links: EvictedQueue::new(20), status, diff --git a/opentelemetry/CHANGELOG.md b/opentelemetry/CHANGELOG.md index e9b313f0d0..d5ebad941c 100644 --- a/opentelemetry/CHANGELOG.md +++ b/opentelemetry/CHANGELOG.md @@ -10,6 +10,14 @@ a `const DEFAULT` value. [#1270](https://github.com/open-telemetry/opentelemetry-rust/pull/1270) - Updated crate documentation and examples. [#1256](https://github.com/open-telemetry/opentelemetry-rust/issues/1256) +- **Breaking** `SpanBuilder` attributes changed from `OrderMap` to + `Vec` and `with_attributes_map` method is removed from `SpanBuilder`. + This implies that OpenTelemetry API will no longer perform + de-dup of attribute Keys. + [#1293](https://github.com/open-telemetry/opentelemetry-rust/issues/1293). + Please share [feedback + here](https://github.com/open-telemetry/opentelemetry-rust/issues/1300), if + you are affected. ## [v0.20.0](https://github.com/open-telemetry/opentelemetry-rust/compare/v0.19.0...v0.20.0) This release should been seen as 1.0-rc3 following 1.0-rc2 in v0.19.0. Refer to CHANGELOG.md in individual creates for details on changes made in different creates. diff --git a/opentelemetry/src/trace/span.rs b/opentelemetry/src/trace/span.rs index 0d101b84bd..97d9578565 100644 --- a/opentelemetry/src/trace/span.rs +++ b/opentelemetry/src/trace/span.rs @@ -110,7 +110,8 @@ pub trait Span { /// Set an attribute of this span. /// /// Setting an attribute with the same key as an existing attribute - /// generally overwrites the existing attribute's value. + /// results in both being stored as attribute, without any de-duplication + /// performed. /// /// Note that the OpenTelemetry project documents certain "[standard /// attributes]" that have prescribed semantic meanings and are available via @@ -123,7 +124,8 @@ pub trait Span { /// Set multiple attributes of this span. /// /// Setting an attribute with the same key as an existing attribute - /// generally overwrites the existing attribute's value. + /// results in both being stored as attribute, without any de-duplication + /// performed. /// /// Note that the OpenTelemetry project documents certain "[standard /// attributes]" that have prescribed semantic meanings and are available via diff --git a/opentelemetry/src/trace/tracer.rs b/opentelemetry/src/trace/tracer.rs index 9fe78b552e..cc8554c216 100644 --- a/opentelemetry/src/trace/tracer.rs +++ b/opentelemetry/src/trace/tracer.rs @@ -1,9 +1,8 @@ use crate::{ trace::{Event, Link, Span, SpanId, SpanKind, Status, TraceContextExt, TraceId, TraceState}, - Context, Key, KeyValue, OrderMap, Value, + Context, KeyValue, }; use std::borrow::Cow; -use std::iter::FromIterator; use std::time::SystemTime; /// The interface for constructing [`Span`]s. @@ -259,8 +258,11 @@ pub struct SpanBuilder { /// Span end time pub end_time: Option, - /// Span attributes - pub attributes: Option>, + /// Span attributes that are provided at the span creation time. + /// More attributes can be added afterwards. + /// Providing duplicate keys will result in multiple attributes + /// with the same key, as there is no de-duplication performed. + pub attributes: Option>, /// Span events pub events: Option>, @@ -326,26 +328,14 @@ impl SpanBuilder { } /// Assign span attributes from an iterable. - /// - /// Check out [`SpanBuilder::with_attributes_map`] to assign span attributes - /// via an [`OrderMap`] instance. + /// Providing duplicate keys will result in multiple attributes + /// with the same key, as there is no de-duplication performed. pub fn with_attributes(self, attributes: I) -> Self where I: IntoIterator, { SpanBuilder { - attributes: Some(OrderMap::from_iter(attributes)), - ..self - } - } - - /// Assign span attributes. - /// - /// Check out [`SpanBuilder::with_attributes`] to assign span attributes - /// from an iterable of [`KeyValue`]s. - pub fn with_attributes_map(self, attributes: OrderMap) -> Self { - SpanBuilder { - attributes: Some(attributes), + attributes: Some(attributes.into_iter().collect()), ..self } }