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

SpanAttributes modified to use Vec instead of OrderMap/EvictedHashMap #1293

Merged
merged 21 commits into from
Oct 20, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 3 additions & 3 deletions opentelemetry-contrib/src/trace/exporter/jaeger_json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,10 +150,10 @@
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);

Check warning on line 154 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#L153-L154

Added lines #L153 - L154 were not covered by tests
serde_json::json!({
"key": key.as_str(),
"key": kv.key.as_str(),

Check warning on line 156 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#L156

Added line #L156 was not covered by tests
"type": tpe,
"value": value,
})
Expand Down
26 changes: 11 additions & 15 deletions opentelemetry-datadog/src/exporter/model/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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")]);
Expand All @@ -233,6 +227,7 @@ pub(crate) mod tests {
start_time,
end_time,
attributes,
dropped_attributes_count: 0,
events,
links,
status: Status::Ok,
Expand Down Expand Up @@ -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\
cijothomas marked this conversation as resolved.
Show resolved Hide resolved
// 1lpHRlc3Snc2VydmljZax0ZXN0LXNlcnZpY2WjZW52qHRlc3QtZW52p3ZlcnNpb26sdGVzdC12ZXJzaW9uqXNwYW4udH\
// lwZbVfc2FtcGxpbmdfcHJpb3JpdHlfdjGRkZzOAAAAAc4AAAACzgAAAAPPAAAAAAAAAAfPAAAAAAAAAGPPAAAAAAAAAA\
// HTAAAAAAAAAADTAAAAADuaygDSAAAAAIXOAAAABM4AAAAFzgAAAAbOAAAAB84AAAAIzgAAAAnOAAAACs4AAAALzgAAAA\
// zOAAAAAIHOAAAADcsAAAAAAAAAAM4AAAAA");

Ok(())
}
Expand Down
25 changes: 16 additions & 9 deletions opentelemetry-datadog/src/exporter/model/v03.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -36,11 +36,18 @@
.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;
}

Check warning on line 47 in opentelemetry-datadog/src/exporter/model/v03.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-datadog/src/exporter/model/v03.rs#L47

Added line #L47 was not covered by tests
}

if !span_type_found {
rmp::encode::write_map_len(&mut encoded, 11)?;
}

Expand Down Expand Up @@ -96,9 +103,9 @@
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")?;
Expand Down
19 changes: 11 additions & 8 deletions opentelemetry-datadog/src/exporter/model/v05.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand Down Expand Up @@ -148,10 +148,13 @@
.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;
}

Check warning on line 156 in opentelemetry-datadog/src/exporter/model/v05.rs

View check run for this annotation

Codecov / codecov/patch

opentelemetry-datadog/src/exporter/model/v05.rs#L156

Added line #L156 was not covered by tests
}

// Datadog span name is OpenTelemetry component name - see module docs for more information
rmp::encode::write_array_len(&mut encoded, SPAN_NUM_ELEMENTS)?;
Expand Down Expand Up @@ -197,9 +200,9 @@

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))?;
Expand Down
23 changes: 11 additions & 12 deletions opentelemetry-jaeger/src/exporter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<KeyValue>,
instrumentation_lib: Option<InstrumentationLibrary>,
status: Status,
kind: SpanKind,
Expand All @@ -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::<Vec<_>>();

Expand Down Expand Up @@ -361,7 +361,6 @@ mod tests {
trace::{SpanKind, Status},
KeyValue,
};
use opentelemetry_sdk::trace::EvictedHashMap;

fn assert_tag_contains(tags: Vec<Tag>, key: &'static str, expect_val: &'static str) {
assert_eq!(
Expand Down Expand Up @@ -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 {
Expand All @@ -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,
));
Expand Down
30 changes: 0 additions & 30 deletions opentelemetry-proto/src/transform/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,21 +55,6 @@ pub mod tonic {
#[derive(Default)]
pub struct Attributes(pub ::std::vec::Vec<crate::proto::tonic::common::v1::KeyValue>);

#[cfg(feature = "trace")]
impl From<opentelemetry_sdk::trace::EvictedHashMap> 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<Vec<opentelemetry::KeyValue>> for Attributes {
fn from(kvs: Vec<opentelemetry::KeyValue>) -> Self {
Attributes(
Expand Down Expand Up @@ -179,21 +164,6 @@ pub mod grpcio {
#[derive(Default)]
pub struct Attributes(pub ::std::vec::Vec<crate::proto::grpcio::common::v1::KeyValue>);

#[cfg(feature = "trace")]
impl From<opentelemetry_sdk::trace::EvictedHashMap> 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<Vec<opentelemetry::KeyValue>> for Attributes {
fn from(kvs: Vec<opentelemetry::KeyValue>) -> Self {
Attributes(
Expand Down
4 changes: 2 additions & 2 deletions opentelemetry-proto/src/transform/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@
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
Expand Down Expand Up @@ -191,7 +191,7 @@
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,

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

View check run for this annotation

Codecov / codecov/patch

opentelemetry-proto/src/transform/trace.rs#L194

Added line #L194 was not covered by tests
attributes: Attributes::from(source_span.attributes).0,
dropped_events_count: source_span.events.dropped_count(),
events: source_span
Expand Down
23 changes: 22 additions & 1 deletion opentelemetry-sdk/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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<KeyValue>` instead of
`EvictedHashMap`. `SpanData` now expose `dropped_attributes_count` as a
separate field.

*Breaking Change Affecting Sampler authors*:

`should_sample` changes `attributes` from `OrderMap<Key, Value>` to
`Vec<KeyValue>`.

### Removed

Expand Down
5 changes: 3 additions & 2 deletions opentelemetry-sdk/benches/batch_span_processor.rs
Original file line number Diff line number Diff line change
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, EvictedHashMap, EvictedQueue, SpanProcessor};
use opentelemetry_sdk::trace::{BatchSpanProcessor, EvictedQueue, SpanProcessor};
use opentelemetry_sdk::Resource;
use std::borrow::Cow;
use std::sync::Arc;
Expand All @@ -27,7 +27,8 @@ fn get_span_data() -> Vec<SpanData> {
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,
Expand Down
22 changes: 1 addition & 21 deletions opentelemetry-sdk/benches/span_builder.rs
Original file line number Diff line number Diff line change
@@ -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::{
Expand Down Expand Up @@ -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();
}

Expand Down
6 changes: 5 additions & 1 deletion opentelemetry-sdk/src/export/trace.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -81,7 +82,10 @@ pub struct SpanData {
/// Span end time
pub end_time: SystemTime,
/// Span attributes
pub attributes: crate::trace::EvictedHashMap,
pub attributes: Vec<KeyValue>,
/// 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<Event>,
/// Span Links
Expand Down
Loading
Loading