Skip to content

Conversation

@Janetxxx
Copy link
Contributor

What I did

Why I did it

How I verified it

Details if related

@Janetxxx Janetxxx requested a review from prsunny as a code owner November 23, 2025 23:40
Copilot AI review requested due to automatic review settings November 23, 2025 23:40
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copilot finished reviewing on behalf of Janetxxx November 23, 2025 23:41
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds OpenTelemetry metrics export functionality and a comprehensive benchmark suite to Countersyncd. The implementation enables SAI statistics to be converted to OpenTelemetry gauge formats and exported to OTLP collectors, along with extensive performance benchmarking capabilities to validate the HFT (High-Frequency Trading) requirements.

Key Changes:

  • New OpenTelemetry message types and conversion pipeline (SAI → OTel → Protobuf)
  • OtelActor for handling metric export with configurable console output and collector endpoints
  • Comprehensive benchmark suite covering message conversions, actor throughput, IPFIX parsing, export performance, memory efficiency, and end-to-end pipelines
  • CLI integration with optional OTel export flags and channel capacity configuration

Reviewed changes

Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
crates/countersyncd/src/message/otel.rs Defines OpenTelemetry data structures and conversion logic from SAI statistics
crates/countersyncd/src/actor/otel.rs Implements OtelActor for metrics export with protobuf conversion and gRPC client
crates/countersyncd/src/main.rs Integrates OTel export with CLI arguments and actor orchestration
crates/countersyncd/benches/*.rs Five comprehensive benchmark suites covering all aspects of the pipeline
crates/countersyncd/Cargo.toml Adds OpenTelemetry, gRPC, and benchmarking dependencies
Cargo.toml Workspace-level dependency additions
Cargo.lock Dependency resolution with version locks

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


// Test Arc cloning cost
group.bench_with_input(
BenchmarkId::new("arc_cloning", format("{}stats", message_size)),
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Missing exclamation mark in format macro call. Should be format! instead of format.

Suggested change
BenchmarkId::new("arc_cloning", format("{}stats", message_size)),
BenchmarkId::new("arc_cloning", format!("{}stats", message_size)),

Copilot uses AI. Check for mistakes.
pub struct OtelActor {
stats_receiver: Receiver<SAIStatsMessage>,
config: OtelActorConfig,
shutdown_notifier: Option<oneshot::Sender<()>>,
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The client field lacks documentation. Consider adding a doc comment explaining that this is a reusable gRPC client for exporting metrics to the OTLP collector.

Suggested change
shutdown_notifier: Option<oneshot::Sender<()>>,
shutdown_notifier: Option<oneshot::Sender<()>>,
/// Reusable gRPC client for exporting metrics to the OTLP collector.

Copilot uses AI. Check for mistakes.
Comment on lines 235 to 236
let observation_time_nano = 0u64; // 1970-01-01 00:00:00 UTC
let data_point = OtelDataPoint::from_sai_stat(&sai_stat, observation_time_nano);
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

The comment '1970-01-01 00:00:00 UTC' is misleading since this is in the test. Consider using a more meaningful test timestamp constant or clarifying this is intentionally epoch zero for testing.

Suggested change
let observation_time_nano = 0u64; // 1970-01-01 00:00:00 UTC
let data_point = OtelDataPoint::from_sai_stat(&sai_stat, observation_time_nano);
// Intentionally using epoch zero (1970-01-01 00:00:00 UTC) as test timestamp.
const TEST_OBSERVATION_TIME_NANO_EPOCH: u64 = 0;
let data_point = OtelDataPoint::from_sai_stat(&sai_stat, TEST_OBSERVATION_TIME_NANO_EPOCH);

Copilot uses AI. Check for mistakes.
counter: 5000,
};

let observation_time_nano = 0u64; // 1970-01-01 00:00:00 UTC
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

Same misleading comment as line 235. Consider defining a test constant like TEST_EPOCH_TIMESTAMP to make the intent clearer.

Copilot uses AI. Check for mistakes.
}
}

pub fn print_conversion_report(sai_stats: &SAIStats, otel_metrics: &OtelMetrics) {
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

This function is declared as pub but is never called in the main code (only used in tests implicitly). Consider making it private or documenting its intended public use case.

Suggested change
pub fn print_conversion_report(sai_stats: &SAIStats, otel_metrics: &OtelMetrics) {
fn print_conversion_report(sai_stats: &SAIStats, otel_metrics: &OtelMetrics) {

Copilot uses AI. Check for mistakes.
/// Enable OpenTelemetry console output
#[arg(
long,
default_value = "true",
Copy link

Copilot AI Nov 23, 2025

Choose a reason for hiding this comment

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

[nitpick] Default value for otel_console is 'true', which means console output is enabled by default when OTel is enabled. This could generate excessive log output in production. Consider defaulting to 'false' for production readiness.

Suggested change
default_value = "true",
default_value = "false",

Copilot uses AI. Check for mistakes.
@prsunny prsunny requested a review from Pterosaur November 24, 2025 22:57
@prsunny
Copy link
Collaborator

prsunny commented Nov 24, 2025

@Janetxxx , please fix the description per template and give details of the change

@Janetxxx Janetxxx marked this pull request as draft November 24, 2025 23:16
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants