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

Preallocate storage for log attributes on stack #1965

Merged
merged 75 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
7fee0fc
inital commit
lalitb Jul 23, 2024
06aee01
more
lalitb Jul 23, 2024
ea7b2f8
more changes
lalitb Jul 23, 2024
bfb1297
changes
lalitb Jul 23, 2024
fcb0ed8
fix build
lalitb Jul 24, 2024
40b2b5f
changes
lalitb Jul 24, 2024
da8b8de
revert defaults
lalitb Jul 24, 2024
5019086
optionally create vec
lalitb Jul 24, 2024
17d1628
add intial capacity for vector
lalitb Jul 24, 2024
781c3ed
create vec of certain capacity
lalitb Jul 25, 2024
b9a90c2
changes
lalitb Jul 25, 2024
2bfde32
Merge branch 'main' into preallocate-attribute-mem
lalitb Jul 25, 2024
a95e681
cont..
lalitb Jul 25, 2024
9dc8a45
changes..
lalitb Jul 26, 2024
df66985
box enums
lalitb Jul 26, 2024
8f62fa3
make growable add inline
lalitb Jul 26, 2024
fb7a44c
update bench
lalitb Jul 26, 2024
8611bc0
Merge branch 'main' into preallocate-attribute-mem
lalitb Jul 27, 2024
2fabca1
remove experimenal metadata as default
lalitb Jul 27, 2024
e52d002
changes.
lalitb Jul 28, 2024
92d8689
Merge branch 'main' into preallocate-attribute-mem
lalitb Jul 28, 2024
877996e
revert back to option
lalitb Jul 29, 2024
8a86be3
Merge branch 'preallocate-attribute-mem' of github.com:lalitb/opentel…
lalitb Jul 29, 2024
283816b
revert the checkedin proto
lalitb Jul 29, 2024
77e1ae1
back
lalitb Jul 29, 2024
ad42e0b
trying again to revert unwanted otel-proto update
lalitb Jul 29, 2024
e99a957
another revert try
lalitb Jul 29, 2024
c97a948
ci errors
lalitb Jul 29, 2024
1adec8c
Merge branch 'main' into preallocate-attribute-mem
TommyCpp Jul 29, 2024
660a6d8
fix tests
lalitb Jul 29, 2024
9e09502
Merge branch 'preallocate-attribute-mem' of github.com:lalitb/opentel…
lalitb Jul 29, 2024
8e0bfaa
Update opentelemetry-sdk/src/logs/growable_array.rs
lalitb Jul 29, 2024
9d44e26
Update Cargo.toml
lalitb Jul 29, 2024
eed39a1
Update Cargo.toml
lalitb Jul 30, 2024
9275a11
review comments
lalitb Jul 31, 2024
8c28428
Merge branch 'main' into preallocate-attribute-mem
lalitb Jul 31, 2024
0f6b699
lint issues
lalitb Jul 31, 2024
1a2ec1f
comments
lalitb Jul 31, 2024
adc48ca
Update opentelemetry-sdk/src/logs/growable_array.rs
lalitb Jul 31, 2024
0a4715a
revert back box
lalitb Aug 1, 2024
b619409
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 1, 2024
6136678
change var names
lalitb Aug 1, 2024
1083e17
Merge remote-tracking branch 'refs/remotes/origin/preallocate-attribu…
lalitb Aug 1, 2024
7575f4b
Merge branch 'main' into preallocate-attribute-mem
TommyCpp Aug 1, 2024
cb0d05d
move growable array outside logs
lalitb Aug 2, 2024
196d2c7
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 2, 2024
c9816f0
review comments
lalitb Aug 2, 2024
2a149f9
use get_or_insert_with
lalitb Aug 2, 2024
3caf8b4
clioppy warning
lalitb Aug 2, 2024
0a876bf
remove growablearray from test
lalitb Aug 5, 2024
a0fc1f3
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 6, 2024
6c468e0
Update lib.rs
lalitb Aug 6, 2024
6f58225
Update lib.rs
lalitb Aug 6, 2024
b7ca124
Update lib.rs
lalitb Aug 6, 2024
846ca89
Update Cargo.toml
lalitb Aug 6, 2024
ee57967
merge conflict
lalitb Aug 6, 2024
f9dbdf0
Merge branch 'main' into preallocate-attribute-mem
cijothomas Aug 7, 2024
8911c39
Update record.rs
lalitb Aug 7, 2024
d500402
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 7, 2024
bbe5689
modify attributes_contain
lalitb Aug 7, 2024
4117af4
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 8, 2024
b256195
Update opentelemetry-sdk/src/growable_array.rs
lalitb Aug 8, 2024
40a97ac
fix
lalitb Aug 8, 2024
085f001
Update opentelemetry-sdk/src/growable_array.rs
lalitb Aug 8, 2024
cb200a5
fix attributes
lalitb Aug 8, 2024
da5a5ab
Merge branch 'preallocate-attribute-mem' of github.com:lalitb/opentel…
lalitb Aug 8, 2024
f3b63fb
fix
lalitb Aug 8, 2024
4fa708b
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 19, 2024
e95d550
merge conflict
lalitb Aug 19, 2024
2e066fc
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 22, 2024
93b4775
fix lint
lalitb Aug 23, 2024
e697b0d
update results
lalitb Aug 23, 2024
9e8871e
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 23, 2024
e516abe
comment
lalitb Aug 23, 2024
90dff08
Merge branch 'main' into preallocate-attribute-mem
lalitb Aug 23, 2024
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
12 changes: 8 additions & 4 deletions opentelemetry-appender-log/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -938,12 +938,16 @@ mod tests {
);

let logs = exporter.get_emitted_logs().unwrap();
let attributes = &logs[0].record.attributes.as_ref().unwrap();
let attributes = &logs[0].record.attributes;

let get = |needle: &str| {
attributes.iter().find_map(|(k, v)| {
if k.as_str() == needle {
Some(v.clone())
attributes.iter().find_map(|attr| {
if let Some((k, v)) = attr {
if k.as_str() == needle {
Some(v.clone())
} else {
None
}
} else {
None
}
Expand Down
1 change: 0 additions & 1 deletion opentelemetry-appender-tracing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ pprof = { version = "0.13", features = ["flamegraph", "criterion"] }
experimental_metadata_attributes = ["dep:tracing-log"]
logs_level_enabled = ["opentelemetry/logs_level_enabled"]


Copy link
Member

Choose a reason for hiding this comment

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

nit: this was unintended?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think so, but it seems better now with a single newline.

[[bench]]
name = "logs"
harness = false
Expand Down
101 changes: 59 additions & 42 deletions opentelemetry-appender-tracing/src/layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,29 +252,35 @@ mod tests {
assert!(log.record.trace_context.is_none());

// Validate attributes
let attributes: Vec<(Key, AnyValue)> = log
.record
.attributes
.clone()
.expect("Attributes are expected");
let attributes = log.record.attributes.clone();
#[cfg(not(feature = "experimental_metadata_attributes"))]
assert_eq!(attributes.len(), 3);
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(attributes.len(), 8);
assert!(attributes.contains(&(Key::new("event_id"), 20.into())));
assert!(attributes.contains(&(Key::new("user_name"), "otel".into())));
assert!(attributes.contains(&(Key::new("user_email"), "[email protected]".into())));
assert!(attributes.contains(&Some((Key::new("event_id"), 20.into()))));
assert!(attributes.contains(&Some((Key::new("user_name"), "otel".into()))));
assert!(attributes.contains(&Some((
Key::new("user_email"),
"[email protected]".into()
))));
#[cfg(feature = "experimental_metadata_attributes")]
{
assert!(attributes.contains(&(Key::new("code.filename"), "layer.rs".into())));
assert!(attributes.contains(&(
assert!(attributes.contains(&Some((Key::new("code.filename"), "layer.rs".into()))));
assert!(attributes.contains(&Some((
Key::new("code.namespace"),
"opentelemetry_appender_tracing::layer::tests".into()
)));
))));
// The other 3 experimental_metadata_attributes are too unstable to check their value.
// Ex.: The path will be different on a Windows and Linux machine.
// Ex.: The line can change easily if someone makes changes in this source file.
let attributes_key: Vec<Key> = attributes.iter().map(|(key, _)| key.clone()).collect();
let attributes_key: Vec<Key> = attributes
.iter()
.filter_map(|attr| match attr {
Some((key, _)) => Some(key.clone()),
None => None,
})
.collect();

assert!(attributes_key.contains(&Key::new("code.filepath")));
assert!(attributes_key.contains(&Key::new("code.lineno")));
assert!(attributes_key.contains(&Key::new("log.target")));
Expand Down Expand Up @@ -348,29 +354,36 @@ mod tests {
);

// validate attributes.
let attributes: Vec<(Key, AnyValue)> = log
.record
.attributes
.clone()
.expect("Attributes are expected");
let attributes = log.record.attributes.clone();
#[cfg(not(feature = "experimental_metadata_attributes"))]
assert_eq!(attributes.len(), 3);
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(attributes.len(), 8);
assert!(attributes.contains(&(Key::new("event_id"), 20.into())));
assert!(attributes.contains(&(Key::new("user_name"), "otel".into())));
assert!(attributes.contains(&(Key::new("user_email"), "[email protected]".into())));
assert!(attributes.contains(&Some((Key::new("event_id"), 20.into()))));
assert!(attributes.contains(&Some((Key::new("user_name"), "otel".into()))));
assert!(attributes.contains(&Some((
Key::new("user_email"),
"[email protected]".into()
))));
#[cfg(feature = "experimental_metadata_attributes")]
{
assert!(attributes.contains(&(Key::new("code.filename"), "layer.rs".into())));
assert!(attributes.contains(&(
assert!(attributes.contains(&Some((Key::new("code.filename"), "layer.rs".into()))));
assert!(attributes.contains(&Some((
Key::new("code.namespace"),
"opentelemetry_appender_tracing::layer::tests".into()
)));
))));
// The other 3 experimental_metadata_attributes are too unstable to check their value.
// Ex.: The path will be different on a Windows and Linux machine.
// Ex.: The line can change easily if someone makes changes in this source file.
let attributes_key: Vec<Key> = attributes.iter().map(|(key, _)| key.clone()).collect();

//let attributes_key: Vec<Key> = attributes.iter().map(|(key, _)| key.clone()).collect();
let attributes_key: Vec<Key> = attributes
.iter()
.filter_map(|attr| match attr {
Some((key, _)) => Some(key.clone()),
None => None,
})
.collect();
assert!(attributes_key.contains(&Key::new("code.filepath")));
assert!(attributes_key.contains(&Key::new("code.lineno")));
assert!(attributes_key.contains(&Key::new("log.target")));
Expand Down Expand Up @@ -415,27 +428,29 @@ mod tests {

// Validate attributes
#[cfg(feature = "experimental_metadata_attributes")]
let attributes: Vec<(Key, AnyValue)> = log
.record
.attributes
.clone()
.expect("Attributes are expected");
let attributes = log.record.attributes.clone();

// Attributes can be polluted when we don't use this feature.
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(attributes.len(), 5);

#[cfg(feature = "experimental_metadata_attributes")]
{
assert!(attributes.contains(&(Key::new("code.filename"), "layer.rs".into())));
assert!(attributes.contains(&(
assert!(attributes.contains(&(Some((Key::new("code.filename"), "layer.rs".into())))));
assert!(attributes.contains(&Some((
Key::new("code.namespace"),
"opentelemetry_appender_tracing::layer::tests".into()
)));
))));
// The other 3 experimental_metadata_attributes are too unstable to check their value.
// Ex.: The path will be different on a Windows and Linux machine.
// Ex.: The line can change easily if someone makes changes in this source file.
let attributes_key: Vec<Key> = attributes.iter().map(|(key, _)| key.clone()).collect();
let attributes_key: Vec<Key> = attributes
.iter()
.filter_map(|attr| match attr {
Some((key, _)) => Some(key.clone()),
None => None,
})
.collect();
assert!(attributes_key.contains(&Key::new("code.filepath")));
assert!(attributes_key.contains(&Key::new("code.lineno")));
assert!(attributes_key.contains(&Key::new("log.target")));
Expand Down Expand Up @@ -511,27 +526,29 @@ mod tests {

// validate attributes.
#[cfg(feature = "experimental_metadata_attributes")]
let attributes: Vec<(Key, AnyValue)> = log
.record
.attributes
.clone()
.expect("Attributes are expected");
let attributes = log.record.attributes.clone();

// Attributes can be polluted when we don't use this feature.
#[cfg(feature = "experimental_metadata_attributes")]
assert_eq!(attributes.len(), 5);

#[cfg(feature = "experimental_metadata_attributes")]
{
assert!(attributes.contains(&(Key::new("code.filename"), "layer.rs".into())));
assert!(attributes.contains(&(
assert!(attributes.contains(&Some((Key::new("code.filename"), "layer.rs".into()))));
assert!(attributes.contains(&Some((
Key::new("code.namespace"),
"opentelemetry_appender_tracing::layer::tests".into()
)));
))));
// The other 3 experimental_metadata_attributes are too unstable to check their value.
// Ex.: The path will be different on a Windows and Linux machine.
// Ex.: The line can change easily if someone makes changes in this source file.
let attributes_key: Vec<Key> = attributes.iter().map(|(key, _)| key.clone()).collect();
let attributes_key: Vec<Key> = attributes
.iter()
.filter_map(|attr| match attr {
Some((key, _)) => Some(key.clone()),
None => None,
})
.collect();
assert!(attributes_key.contains(&Key::new("code.filepath")));
assert!(attributes_key.contains(&Key::new("code.lineno")));
assert!(attributes_key.contains(&Key::new("log.target")));
Expand Down
18 changes: 13 additions & 5 deletions opentelemetry-proto/src/transform/logs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,18 +93,26 @@ pub mod tonic {
severity_text: log_record.severity_text.map(Into::into).unwrap_or_default(),
body: log_record.body.map(Into::into),
attributes: {
let mut attributes = log_record
let mut attributes: Vec<KeyValue> = log_record
.attributes
.map(Attributes::from_iter)
.unwrap_or_default()
.0;
.iter()
.filter_map(|kv| {
kv.as_ref().map(|(k, v)| KeyValue {
key: k.to_string(),
value: Some(AnyValue {
value: Some(v.clone().into()),
}),
})
})
.collect();

if let Some(event_name) = log_record.event_name.as_ref() {
attributes.push(KeyValue {
key: "name".into(),
value: Some(AnyValue {
value: Some(Value::StringValue(event_name.to_string())),
}),
})
});
}
attributes
},
Expand Down
11 changes: 11 additions & 0 deletions opentelemetry-sdk/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ rustdoc-args = ["--cfg", "docsrs"]
[dev-dependencies]
criterion = { workspace = true, features = ["html_reports"] }
temp-env = { workspace = true }
jemallocator = { version = "0.5"}
jemalloc-ctl = { version = "0.5"}


[target.'cfg(not(target_os = "windows"))'.dev-dependencies]
pprof = { version = "0.13", features = ["flamegraph", "criterion"] }
Expand All @@ -51,6 +54,9 @@ testing = ["opentelemetry/testing", "trace", "metrics", "logs", "rt-async-std",
rt-tokio = ["tokio", "tokio-stream"]
rt-tokio-current-thread = ["tokio", "tokio-stream"]
rt-async-std = ["async-std"]
memory-profiling = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be a crate feature?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, leftover of mem stats I though of adding earlier. Removed now.




[[bench]]
name = "context"
Expand Down Expand Up @@ -91,3 +97,8 @@ required-features = ["metrics"]
name = "log"
harness = false
required-features = ["logs"]

Copy link
Member

Choose a reason for hiding this comment

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

nit: unintentional ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it was, removed the newline now.

[[bench]]
name = "growable_array"
harness = false
required-features = ["logs"]
97 changes: 97 additions & 0 deletions opentelemetry-sdk/benches/growable_array.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
use criterion::{criterion_group, criterion_main, Criterion};
use opentelemetry::logs::AnyValue;
use opentelemetry::Key;
use opentelemetry_sdk::logs::GrowableArray;

#[derive(Clone, Debug, PartialEq)]
pub struct KeyValuePair(Key, AnyValue);

impl Default for KeyValuePair {
fn default() -> Self {
KeyValuePair(Key::from_static_str(""), AnyValue::Int(0))
}
}

fn growable_array_insertion_benchmark(c: &mut Criterion) {
c.bench_function("GrowableArray Insertion", |b| {
b.iter(|| {
let mut collection = GrowableArray::<KeyValuePair>::new();
for i in 0..8 {
let key = Key::from(format!("key{}", i));
let value = AnyValue::Int(i as i64);
collection.push(KeyValuePair(key, value));
}
})
});
}

fn vec_insertion_benchmark(c: &mut Criterion) {
c.bench_function("Vec Insertion", |b| {
b.iter(|| {
let mut collection = Vec::with_capacity(10);
for i in 0..8 {
let key = Key::from(format!("key{}", i));
let value = AnyValue::Int(i as i64);
collection.push(KeyValuePair(key, value));
}
})
});
}

fn growable_array_iteration_benchmark(c: &mut Criterion) {
c.bench_function("GrowableArray Iteration", |b| {
let mut collection = GrowableArray::<KeyValuePair>::new();
for i in 0..8 {
let key = Key::from(format!("key{}", i));
let value = AnyValue::Int(i as i64);
collection.push(KeyValuePair(key, value));
}
b.iter(|| {
for item in &collection {
criterion::black_box(item);
}
})
});
}

fn growable_array_get_benchmark(c: &mut Criterion) {
c.bench_function("GrowableArray Get Loop", |b| {
let mut collection = GrowableArray::<KeyValuePair>::new();
for i in 0..8 {
let key = Key::from(format!("key{}", i));
let value = AnyValue::Int(i as i64);
collection.push(KeyValuePair(key, value));
}
b.iter(|| {
for i in 0..collection.len() {
criterion::black_box(collection.get(i));
}
})
});
}

fn vec_iteration_benchmark(c: &mut Criterion) {
c.bench_function("Vec Iteration", |b| {
let mut collection = Vec::with_capacity(10);
for i in 0..8 {
let key = Key::from(format!("key{}", i));
let value = AnyValue::Int(i as i64);
collection.push(KeyValuePair(key, value));
}
b.iter(|| {
for item in &collection {
criterion::black_box(item);
}
})
});
}

criterion_group!(
benches,
growable_array_insertion_benchmark,
vec_insertion_benchmark,
growable_array_iteration_benchmark,
growable_array_get_benchmark,
vec_iteration_benchmark
);
criterion_main!(benches);
Loading
Loading