Skip to content

feat(udp-notif): improve codec with configurable reassembly limits, align to draft, and add OTel metrics - #18

Open
rodonile wants to merge 5 commits into
network-analytics:mainfrom
rodonile:udp-notif-improv
Open

feat(udp-notif): improve codec with configurable reassembly limits, align to draft, and add OTel metrics#18
rodonile wants to merge 5 commits into
network-analytics:mainfrom
rodonile:udp-notif-improv

Conversation

@rodonile

Copy link
Copy Markdown
Member

Changelog

Reassembly hardening

  • Duplicate segment drop (with counter)
  • Configurable per-message segment cap (max_segments, default 64)
  • Configurable reassembly timeout (reassembly_timeout, default 10 s) — eviction runs inside decode()
  • ReassemblyEventCounts struct + take_reassembly_events() helper for draining counters to telemetry

Protocol correctness

  • check_len now enforces buf.len() == message_length (exact match, per draft §3.2 — each datagram is self-contained)
  • udp-notif options collected from first segment only, per draft §4.1
  • Removed in_message: bool stream-codec state (not relevant for UDP)

Observability
Four new OTel instruments on the actor: reassembly.timeout_evictions, reassembly.duplicate_drops, reassembly.max_segments_exceeded (counters), reassembly.incomplete_messages (gauge).

Config propagation
reassembly_max_segments / reassembly_timeout wired from UdpNotifConfigSupervisorConfigActorHandle → per-peer UdpPacketCodec::new(…). Both fields are optional in YAML with defaults from the draft suggestions.

Tests
7 new unit tests in codec.rs. Replaced the incorrect stream-accumulation test in pcap-decoder with a datagram-truncation test that matches the new behaviour.

Copilot AI left a comment

Copy link
Copy Markdown

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 hardens the UDP-Notif packet codec’s reassembly logic, aligns datagram-length validation with the draft (each UDP datagram is self-contained), and propagates new reassembly configuration + OpenTelemetry metrics through the udp-notif service stack.

Changes:

  • Adds configurable reassembly limits (max segments per message, reassembly timeout) and wires them from collector config → supervisor → actor → per-peer codec.
  • Updates codec behavior for protocol correctness (exact message-length match; options taken from first segment) and adds unit tests.
  • Adds OTel instruments for reassembly events (timeouts, duplicates, max-segments exceeded) and an incomplete-messages gauge.

Reviewed changes

Copilot reviewed 11 out of 16 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/udp-notif-service/src/supervisor.rs Adds reassembly settings to supervisor config defaults and passes them into actor construction.
crates/udp-notif-service/src/actor.rs Plumbs reassembly config into per-peer codecs and records new OTel reassembly metrics/gauge.
crates/udp-notif-service/examples/udp-notif-print.rs Switches tracing filtering to RUST_LOG via EnvFilter.
crates/udp-notif-service/examples/udp-notif-print-decoded.rs New example that decodes and prints fully-decoded UDP-Notif payloads with reassembly event logging.
crates/udp-notif-service/examples/udp-notif-actors.rs Switches tracing filtering to RUST_LOG via EnvFilter.
crates/udp-notif-service/examples/udp-notif-actor.rs Updates actor example to pass reassembly defaults into ActorHandle::new.
crates/udp-notif-service/examples/udp-notif-actor-decoded.rs New actor example that decodes payloads to JSON and includes periodic peer purging.
crates/udp-notif-pkt/src/wire/test/pcap_tests.rs Updates error text to reflect codec-level errors.
crates/udp-notif-pkt/src/codec.rs Implements configurable reassembly limits, timeout eviction logic, strict datagram length checks, and adds unit tests + telemetry drain helper.
crates/pcap-decoder/src/handlers/udp_notif.rs Updates test to treat truncated datagrams as malformed (no cross-datagram accumulation).
crates/collector/src/config.rs Adds YAML-configurable defaults for reassembly max segments and timeout.
assets/pcaps/udp-notif/huawei/huawei-vrp-r25c10-udp-notif.json Adds updated decoded output fixture data for UDP-Notif pcap assets.

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

Comment thread crates/udp-notif-pkt/src/codec.rs
Comment thread crates/udp-notif-pkt/src/codec.rs
@rodonile
rodonile force-pushed the udp-notif-improv branch 2 times, most recently from 2167fae to 683c133 Compare July 30, 2026 14:29
@rodonile
rodonile enabled auto-merge (rebase) July 30, 2026 14:29
@rodonile
rodonile force-pushed the udp-notif-improv branch 2 times, most recently from 7224908 to d97b6f7 Compare July 31, 2026 06:19
Comment thread crates/udp-notif-service/src/actor.rs
Comment thread crates/udp-notif-service/src/actor.rs
Comment thread crates/udp-notif-service/src/actor.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 16 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

crates/udp-notif-service/examples/udp-notif-print-decoded.rs:2

  • The file header contains a duplicated copyright line.
// Copyright (C) 2026-present The NetCalyx Authors.
// Copyright (C) 2026-present The NetCalyx Authors.

crates/udp-notif-pkt/src/codec.rs:324

  • UdpPacketCodec::new accepts max_segments: u16 but does not guard against 0. With max_segments == 0, the current cap check effectively allows the first segment (because the buffer doesn't exist yet) and then fails on the second segment, which is surprising and makes the limit behave like 1.

Clamping to a minimum of 1 at construction keeps the semantics consistent and avoids odd edge cases from config parsing.

    pub fn new(max_segments: u16, reassembly_timeout: Duration) -> Self {
        Self {
            incomplete_messages: HashMap::new(),
            max_segments,
            reassembly_timeout,
            reassembly_events: ReassemblyEventCounts::default(),
        }

Comment thread crates/udp-notif-service/src/actor.rs
@rodonile
rodonile force-pushed the udp-notif-improv branch 2 times, most recently from de73a05 to b01705d Compare July 31, 2026 14:59
@rodonile
rodonile requested a review from Copilot July 31, 2026 15:00

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 11 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (4)

crates/udp-notif-service/src/actor.rs:906

  • Reassembly-timeout eviction in UdpPacketCodec runs inside decode(). For peers that go silent mid-reassembly, reassembly_timeout (default 10s) won’t be enforced (and timeout_evictions won’t be emitted) until the next datagram from that peer or until the much-larger idle peer purge runs. If strict timeout enforcement/telemetry is required, consider adding a periodic sweep that drives eviction at ~reassembly_timeout granularity.
        // Purges peers idle for longer than AUTO_PEER_PURGE_INTERVAL
        let mut peer_purge_interval = tokio::time::interval(AUTO_PEER_PURGE_INTERVAL);
        peer_purge_interval.set_missed_tick_behavior(tokio::time::MissedTickBehavior::Delay);

crates/udp-notif-service/examples/udp-notif-print-decoded.rs:2

  • The new example has the copyright header line duplicated, which is likely unintentional noise in generated license headers.
// Copyright (C) 2026-present The NetCalyx Authors.
// Copyright (C) 2026-present The NetCalyx Authors.

crates/udp-notif-service/src/actor.rs:139

  • AUTO_PEER_PURGE_INTERVAL is used as both the timer tick interval and the inactivity threshold. Because the purge only runs periodically and uses since > dur, peers may remain around for up to almost 2× this interval; the current doc comment reads like a hard maximum.

This issue also appears on line 903 of the same file.

/// How long a peer can go completely silent before its per-peer
/// [`UdpPacketCodec`] (and any reassembly buffer still pending in it) is
/// dropped automatically, independent of the external
/// [`ActorCommand::PurgeUnusedPeers`] command.
const AUTO_PEER_PURGE_INTERVAL: Duration = Duration::from_secs(600);

crates/udp-notif-service/src/actor.rs:826

  • When purging an idle peer, the per-peer reassembly_incomplete_messages gauge is never reset. If the peer had pending buffers, the last recorded non-zero gauge value will linger even though the codec was dropped.
        for peer in &cleared_peers {
            if let Some(codec) = self.clients.remove(peer) {
                let incomplete_count = codec.incomplete_messages_count();
                if incomplete_count > 0 {
                    debug!(

@rodonile
rodonile requested a review from riccardo-negri July 31, 2026 15:20
rodonile added 2 commits July 31, 2026 17:44
decode() only evicted timed-out reassembly buffers when the same peer
sent more data, so a peer that stopped sending mid-reassembly left its
per-peer codec (and any pending buffer) in memory forever.

- actor.rs: run() now calls purge_unused_peers automatically every
  10 minutes, independent of inbound traffic or external callers.
  purge_unused_peers is now the single place peers/codecs
  are dropped, and logs any pending buffer discarded with them.
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