[countersyncd] OtelActor performance enhancements #4781
Conversation
Signed-off-by: Janet Cui <janet970527@gmail.com>
Signed-off-by: Janet Cui <janet970527@gmail.com>
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR updates countersyncd’s OpenTelemetry (OTLP) export path and benchmarking setup to reduce overhead during retries and to produce more accurate performance measurements/profiling output.
Changes:
- Refactors
OtelActorexport flow to rebuild the OTLP request from the buffer on each retry and avoids cloning the request for export attempts. - Updates the OTEL actor performance benchmark to pre-build input messages outside the timed window and switches Criterion batching to
PerIteration. - Enables debug symbols for bench builds (for profiler symbolization) and ignores
perf.data*artifacts.
Reviewed changes
Copilot reviewed 3 out of 9 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| crates/countersyncd/src/actor/otel.rs | Refactors export request construction and retry behavior to avoid request cloning and keep the buffer intact during retries. |
| crates/countersyncd/benches/otel_actor_perf.rs | Adjusts benchmark setup to exclude message construction from the timed path and changes batching strategy. |
| Cargo.toml | Enables debug symbols for the bench profile to improve profiler output. |
| .gitignore | Adds ignore pattern for perf.data* profiler artifacts. |
Comments suppressed due to low confidence (1)
crates/countersyncd/src/actor/otel.rs:330
- The match arm uses an identifier pattern (
_none) which matches any remaining value; here it effectively meansNone, but it’s less explicit and could hide mistakes if the return type ever changes. Prefer matchingNonedirectly for clarity.
let client = match self.get_client() {
Some(c) => c, // Use existing or newly created client
_none => { // Failed to create client
self.client = None;
self.backoff(attempt).await; // Wait before retrying
3b6f378 to
7ec4eaa
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
crates/countersyncd/src/actor/otel.rs:363
- This PR changes OtelActor’s export/retry behavior by introducing
build_export_request()and rebuilding the OTLP request per attempt, but there doesn’t appear to be any automated test coverage for OtelActor (the existingcrates/countersyncd/tests/*exercise other actors only). Adding a focused tokio test (e.g., using a mock OTLP server like the bench does) would help prevent regressions around retry / empty-buffer handling.
/// Build an export request from the currently buffered metrics.
/// Returns `None` when there is nothing to export (empty metrics).
/// The buffer is left intact so it can be rebuilt on retry.
fn build_export_request(&self) -> Option<ExportMetricsServiceRequest> {
let mut proto_metrics: Vec<Metric> = Vec::new();
for otel_metrics in &self.buffer {
for gauge in &otel_metrics.gauges {
let proto_data_points = gauge.data_points.iter()
Signed-off-by: Janet Cui <janet970527@gmail.com>
7ec4eaa to
d9e5651
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| // Ensure a client can be created before doing request | ||
| // construction; when get_client() fails the build below is skipped. | ||
| if self.get_client().is_none() { | ||
| self.client = None; | ||
| self.backoff(attempt).await; // Wait before retrying |
Signed-off-by: Janet Cui <janet970527@gmail.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/countersyncd/src/actor/otel.rs:321
- send_request() now ensures a client before checking whether there is anything exportable. Because handle_stats_message() unconditionally pushes OtelMetrics into the buffer (even when stats.stats is empty), flush_buffer() can call send_request() with buffered_counters == 0 / no proto metrics, causing endpoint parsing + client/channel creation work even though build_export_request() will return None and nothing is sent. Add a cheap early return before get_client() to skip client creation when no counters were buffered (or equivalently check build_export_request() before get_client()).
async fn send_request(&mut self) -> Result<(), Box<dyn ExportError>> {
for attempt in 1..=MAX_EXPORT_RETRIES {
// Ensure a client can be created before doing request
// construction; when get_client() fails the build below is skipped.
if self.get_client().is_none() {
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…ributes Signed-off-by: Janet Cui <janet970527@gmail.com>
84c60af to
34b3a36
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
| proto_metrics.push(Metric { | ||
| name: gauge.name.clone(), | ||
| description: gauge.description.clone(), | ||
| name: gauge.name.to_string(), | ||
| description: gauge.description.to_string(), | ||
| metadata: vec![], | ||
| data: Some(opentelemetry_proto::tonic::metrics::v1::metric::Data::Gauge(proto_gauge)), |
What I did
Why I did it
How I verified it
Details if related