-
Notifications
You must be signed in to change notification settings - Fork 45
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
Improve efficiency of ETW metrics exporter #134
Merged
cijothomas
merged 31 commits into
open-telemetry:main
from
mattbodd:dev/mboddewy/add_criterion_benchmarks
Jan 25, 2025
+264
−268
Merged
Changes from 28 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
255952c
Initial criterion support
mattbodd 64255ab
Add doc comment to etw bench
mattbodd 7d2890f
Add useful exporter benchmark
mattbodd c327437
Add useful etw benchmark
mattbodd 391d151
Add system info and exporter and etw times
mattbodd fe5e7d0
Run cargo fmt
mattbodd bada294
Update benchmark
mattbodd 18f550a
Try to be more efficient in exporter
mattbodd 8dd5df6
Apply clippy lints
mattbodd 7b18b01
Create ExportMetricsServiceRequest for each metric and replace the da…
mattbodd d167101
Reorder imports
mattbodd 6f3e6f1
Merge branch 'main' into dev/mboddewy/add_criterion_benchmarks
mattbodd afac194
Attempt to bench mark export function more explicitly
mattbodd 4bb3bf1
Use tokio async with criterion
mattbodd 2044d73
Merge main
mattbodd 3bbe76a
Do no create a new exporter for each exporter benchmark
mattbodd c817fa2
Remove etw benchmark and revert etw module to be private
mattbodd 4b995d1
Remove etw benchmark from Cargo.toml
mattbodd 36102a8
Use 10 metrics in the ResourceMetrics that is exported
mattbodd 8b39545
Reuse the same ResourceMetric for every benchmark loop iteration in e…
mattbodd 651913e
Avoid unecessary let (clippy lint)
mattbodd b29b530
Manage own async runtime instead of relying on criterion's
mattbodd 72a6bc9
Move criterion to dev-dependencies in etw-metrics
mattbodd da6a7e4
Merge branch 'main' into dev/mboddewy/add_criterion_benchmarks
lalitb 128490a
Use prost's Message::encoded_len method when allocating the encoding …
mattbodd 1fe84ff
Share mutable vector between exit_export_metric_service_request invoc…
mattbodd 632a02f
Update average bench runtime
mattbodd 47a941d
Merge branch 'main' into dev/mboddewy/add_criterion_benchmarks
lalitb 33c7613
Rerun CI checks to retry flaky lint
mattbodd d898ec9
Add todo to reuse encoding_buffer during export
mattbodd 33889e3
Merge branch 'main' into dev/mboddewy/add_criterion_benchmarks
lalitb File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
//! run with `$ cargo bench --bench exporter -- --exact <test_name>` to run specific test for logs | ||
//! So to run test named "fibonacci" you would run `$ cargo bench --bench exporter -- --exact fibonacci` | ||
//! To run all tests for logs you would run `$ cargo bench --bench exporter` | ||
//! | ||
/* | ||
The benchmark results: | ||
criterion = "0.5.1" | ||
OS: Windows 11 Enterprise N, 23H2, Build 22631.4460 | ||
Hardware: Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz 2.79 GHz, 16vCPUs | ||
RAM: 64.0 GB | ||
| Test | Average time| | ||
|--------------------------------|-------------| | ||
| exporter | 22.203 ms | | ||
*/ | ||
|
||
use opentelemetry::{InstrumentationScope, KeyValue}; | ||
use opentelemetry_etw_metrics::MetricsExporter; | ||
|
||
use opentelemetry_sdk::{ | ||
metrics::{ | ||
data::{DataPoint, Metric, ResourceMetrics, ScopeMetrics, Sum}, | ||
exporter::PushMetricExporter, | ||
Temporality, | ||
}, | ||
Resource, | ||
}; | ||
|
||
use criterion::{criterion_group, criterion_main, Criterion}; | ||
|
||
async fn export(exporter: &MetricsExporter, resource_metrics: &mut ResourceMetrics) { | ||
exporter.export(resource_metrics).await.unwrap(); | ||
} | ||
|
||
fn create_resource_metrics() -> ResourceMetrics { | ||
// Metric does not implement clone so this helper function is used to create a metric | ||
fn create_metric() -> Metric { | ||
let data_point = DataPoint { | ||
attributes: vec![KeyValue::new("datapoint key", "datapoint value")], | ||
start_time: Some(std::time::SystemTime::now()), | ||
time: Some(std::time::SystemTime::now()), | ||
value: 1.0_f64, | ||
exemplars: vec![], | ||
}; | ||
|
||
let sum: Sum<f64> = Sum { | ||
data_points: vec![data_point.clone(); 2_000], | ||
temporality: Temporality::Delta, | ||
is_monotonic: true, | ||
}; | ||
|
||
Metric { | ||
name: "metric_name".into(), | ||
description: "metric description".into(), | ||
unit: "metric unit".into(), | ||
data: Box::new(sum), | ||
} | ||
} | ||
|
||
ResourceMetrics { | ||
resource: Resource::new(vec![KeyValue::new("service.name", "my-service")]), | ||
scope_metrics: vec![ScopeMetrics { | ||
scope: InstrumentationScope::default(), | ||
metrics: vec![ | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
create_metric(), | ||
], | ||
}], | ||
} | ||
} | ||
|
||
fn criterion_benchmark(c: &mut Criterion) { | ||
let runtime = tokio::runtime::Builder::new_multi_thread() | ||
.worker_threads(1) | ||
.enable_all() | ||
.build() | ||
.unwrap(); | ||
|
||
c.bench_function("export", move |b| { | ||
b.iter_custom(|iters| { | ||
let exporter = MetricsExporter::new(); | ||
|
||
let mut resource_metrics = create_resource_metrics(); | ||
|
||
let start = std::time::Instant::now(); | ||
|
||
for _i in 0..iters { | ||
runtime.block_on(async { | ||
export(&exporter, &mut resource_metrics).await; | ||
}); | ||
} | ||
start.elapsed() | ||
}) | ||
}); | ||
} | ||
|
||
criterion_group!(benches, criterion_benchmark); | ||
criterion_main!(benches); |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to micro benchmark just the serialization and export of one data point instead of 20000. It would keep the calculations simple when comparing two benchmark results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is coming from - #134 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let me know if there's a preference between a micro benchmark and what Cijo suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker. Exporting Metrics is not in hot path, so it may not be worth perfecting this.