hftelorch: minor improvements and cleanups for high frequency telemetry by Pterosaur · Pull Request #4315 · sonic-net/sonic-swss

Pterosaur · 2026-03-09T23:05:22Z

What I did
A collection of minor improvements and cleanups for the high frequency telemetry orchagent code:

Add handleSaiCreateStatus() calls in createNetlinkChannel() for consistency with createTAM().
Add missing attrs.push_back() for SAI_TAM_REPORT_ATTR_REPORT_INTERVAL_UNIT.
Use find() instead of operator[] in clearGroup() to avoid inserting default entries.
Adjust clearGroup() cleanup order to delete counter subscriptions before telemetry type/report objects.
Replace raw const char* fields in CounterNameMapUpdater::Message with owned std::string state for robustness.
Initialize CounterNameMapUpdater::Message::m_oid to SAI_NULL_OBJECT_ID and keep it SET-only by contract.
Use rvalue reference for updateStatsIDs() to enable true move semantics.
Use u32 for SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID to match the SAI metadata type.
Clean up partially created hostif objects in createNetlinkChannel() if a later HOSTIF create step fails.
Remove unused CONSTANTS_FILE macro and clean up commented-out code.

Why I did it
Identified during code review. Most of these are defensive robustness/maintainability fixes. The createNetlinkChannel() follow-up also improves failure-path behavior by avoiding leaked partially created HOSTIF objects when later create steps fail.

How I verified it
Code review only. CI will validate build and unit tests.

Details if related
N/A

Remove the unused CONSTANTS_FILE macro with a typo in path. Add handleSaiCreateStatus() calls to createNetlinkChannel() for create_hostif, create_hostif_user_defined_trap, and create_hostif_table_entry. Previously these SAI calls did not check return values, which could lead to using invalid object IDs if creation failed. Also remove commented-out trap group code. Signed-off-by: Ze Gan <[email protected]>

The SAI_TAM_REPORT_ATTR_REPORT_INTERVAL_UNIT attribute was set but never pushed to the attrs vector, so it was not passed to the SAI create call. While the SAI default happens to be USEC, this should be explicitly set for correctness. Signed-off-by: Ze Gan <[email protected]>

clearGroup() used operator[] to access m_sai_tam_tel_type_objs which inserts a default entry when the key doesn't exist, potentially polluting the map. Use find() instead and only erase the state entry when the key is actually present. Signed-off-by: Ze Gan <[email protected]>

Replace the union of SetPayload/DelPayload (containing raw const char* pointers) with a std::string member that owns the counter name. This prevents potential dangling pointer issues if the caller's local string goes out of scope before the message is fully processed. Update locallyNotify() in HFTelOrch to use the new Message fields. Signed-off-by: Ze Gan <[email protected]>

Change updateStatsIDs() parameter from const reference to rvalue reference to enable true move semantics. The previous code used std::move on a const reference which silently fell back to copy. Update callers in HFTelProfile::setStatsIDs() to std::move the local set into the call. Signed-off-by: Ze Gan <[email protected]>

mssonicbld · 2026-03-09T23:12:36Z

/azp run

mssonicbld · 2026-03-09T23:12:36Z

/azp run

azure-pipelines · 2026-03-09T23:12:46Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2026-03-09T23:12:46Z

Azure Pipelines successfully started running 1 pipeline(s).

SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID is defined as sai_uint32_t in the SAI metadata. Use attr.value.u32 instead of attr.value.oid when creating TAM counter subscription objects. Signed-off-by: Ze Gan <[email protected]>

Adjust clearGroup() cleanup order to erase TAM counter subscription objects before removing the TAM telemetry type and report objects. This better matches the object dependency order during teardown. Signed-off-by: Ze Gan <[email protected]>

mssonicbld · 2026-03-10T00:23:51Z

/azp run

azure-pipelines · 2026-03-10T00:24:00Z

Azure Pipelines successfully started running 1 pipeline(s).

banidoru

Clean PR with several meaningful bug fixes. No blocking issues found.

Key fixes verified:

Dangling pointer fix in Message struct (union of const char* → std::string)
Missing attrs.push_back(attr) for report interval unit attribute
Type mismatch fix (oid → u32) for stat_id
Safe lookup in clearGroup preventing default-insertion via operator[]
Added handleSaiCreateStatus error checking on previously unchecked SAI calls
Removed dead code and unused macro with typo (/et/sonic/)

LGTM.

orchagent/high_frequency_telemetry/counternameupdater.h

orchagent/high_frequency_telemetry/hftelprofile.cpp

orchagent/high_frequency_telemetry/hftelgroup.h

banidoru

Clean set of fixes — the dangling-pointer fix, missing push_back, stat_id type correction, and defensive clearGroup are all solid. A few observations below, none blocking.

orchagent/high_frequency_telemetry/counternameupdater.h

orchagent/high_frequency_telemetry/hftelorch.cpp

banidoru

Clean improvements — good bug fixes (missing push_back, oid→u32 type mismatch, clearGroup defensive lookup) and solid safety improvement replacing the union with std::string. One concern about partial-creation cleanup in createNetlinkChannel; otherwise LGTM.

orchagent/high_frequency_telemetry/hftelorch.cpp

banidoru

All reviewers approved. LGTM.

Copilot

Pull request overview

Updates the orchagent high-frequency telemetry (HFT) implementation with small robustness, correctness, and cleanup improvements across profile/group management, SAI TAM object creation, and counter-name update plumbing.

Changes:

Improve TAM/hostif creation and report configuration correctness (add missing report-interval-unit attribute; use handleSaiCreateStatus() for hostif creates).
Make group/profile bookkeeping safer (avoid operator[] insertion in clearGroup(), adjust cleanup ordering; enable actual move semantics for stats ID updates).
Harden counter-name update messaging by owning the counter name (std::string) instead of using non-owning const char* payloads.

Reviewed changes

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

Show a summary per file

File	Description
orchagent/high_frequency_telemetry/hftelprofile.cpp	Moves stats ID sets into groups, fixes `clearGroup()` map handling/cleanup order, and pushes missing report interval unit attribute; adjusts counter subscription stat-id attribute type.
orchagent/high_frequency_telemetry/hftelorch.cpp	Uses `handleSaiCreateStatus()` for hostif object creations and updates local notification handling to use the new message shape.
orchagent/high_frequency_telemetry/hftelgroup.h	Changes `updateStatsIDs()` signature to accept an rvalue set to enable move semantics.
orchagent/high_frequency_telemetry/hftelgroup.cpp	Implements moved `updateStatsIDs()` and uses `std::move` into the member set.
orchagent/high_frequency_telemetry/counternameupdater.h	Replaces union payload with owned `std::string` counter name + `m_oid` field.
orchagent/high_frequency_telemetry/counternameupdater.cpp	Updates message construction accordingly and removes dependence on temporary-string `c_str()` lifetimes.

You can also share your feedback on Copilot code review. Take the survey.

orchagent/high_frequency_telemetry/counternameupdater.cpp

orchagent/high_frequency_telemetry/hftelorch.cpp

Signed-off-by: Ze Gan <[email protected]>

banidoru

All fixes look correct and well-structured. Key changes verified:

Dangling pointer fix: Message struct now owns data via std::string for both m_table_name and m_counter_name. Union eliminated.
Missing push_back: SAI_TAM_REPORT_ATTR_REPORT_INTERVAL_UNIT now correctly added to attrs vector.
Type mismatch: SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID correctly uses u32 instead of oid.
clearGroup safety: find() replaces operator[] preventing spurious map insertions.
Partial failure cleanup: createNetlinkChannel() now checks each SAI create and calls deleteNetlinkChannel() on failure.
Move semantics: updateStatsIDs() rvalue-ref parameter enables true move instead of silent copy.
Dead code removal: Unused CONSTANTS_FILE macro (with typo) and commented-out code removed.

No new issues found.

banidoru

Clean set of bug fixes for the high-frequency telemetry subsystem. The latest iteration addresses all prior feedback well.

Key fixes verified:

Message struct now owns its strings via std::string, eliminating dangling-pointer hazards from the old const char*/union design
Missing attrs.push_back(attr) for SAI_TAM_REPORT_ATTR_REPORT_INTERVAL_UNIT — silent data loss bug
attr.value.oid → attr.value.u32 for SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID — type mismatch fix
clearGroup defensive find-before-erase prevents accidental default-insertion into the map
createNetlinkChannel now checks return status and calls deleteNetlinkChannel() on partial failure
Dead code removal (CONSTANTS_FILE macro with typo, commented-out trap group code)
Move semantics for updateStatsIDs — correct and all call sites use std::move

No new issues found. LGTM.

banidoru

Solid bug-fix PR. All changes are correct and well-targeted:

Dangling pointer fix (Message struct): Replacing raw const char* + union with owning std::string members eliminates a real use-after-scope hazard. Default-initializing m_oid to SAI_NULL_OBJECT_ID is clean.
Missing attrs.push_back for report interval unit: Clear bug — the attribute was configured but never added to the vector.
stat_id type mismatch (oid → u32): Correct per SAI spec; SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID is a u32 enum, not an OID.
clearGroup defensive lookup: Using find-then-erase instead of operator[] prevents silent default-insertion of a zero OID key.
createNetlinkChannel error handling: Checking handleSaiCreateStatus return and calling deleteNetlinkChannel() on partial failure prevents SAI object leaks.
Move semantics for updateStatsIDs: Appropriate since all call sites build a temporary set.
Dead code removal: Typo-laden CONSTANTS_FILE macro and commented-out trap group code cleaned up.

LGTM.

banidoru

All reviewers approved. LGTM.

banidoru

Solid bugfix PR. All issues from prior review rounds have been addressed:

Dangling pointer fix: Message struct now owns data via std::string for both m_table_name and m_counter_name. m_oid properly default-initialized.
Missing attrs.push_back: SAI_TAM_REPORT_ATTR_REPORT_INTERVAL_UNIT was silently dropped — now correctly pushed.
Type mismatch: SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID correctly uses u32 instead of oid.
clearGroup defensive lookup: operator[] replaced with find() to avoid spurious default-insertion.
createNetlinkChannel error handling: SAI create calls now checked with handleSaiCreateStatus and rollback via deleteNetlinkChannel on partial failure.
Move semantics: updateStatsIDs correctly takes rvalue ref, callers use std::move.
Dead code cleanup: Removed unused CONSTANTS_FILE macro and commented-out code.

No new issues found. LGTM.

banidoru

All fixes look correct. The dangling-pointer hazard (union of raw const char* → owning std::string members) is properly resolved, m_oid has a safe default initializer, the missing attrs.push_back for the report interval unit is fixed, the stat_id type mismatch (oid → u32) is corrected, the defensive find-before-erase in clearGroup prevents accidental default-insertion, and createNetlinkChannel now properly checks SAI return status with rollback via deleteNetlinkChannel() on partial failure. Clean set of bug fixes.

banidoru

Solid set of bug fixes. The union-to-struct migration eliminates dangling pointer hazards, the missing attrs.push_back for report interval unit was a real bug, the u32 vs oid type correction aligns with SAI spec, the defensive find before erase in clearGroup prevents silent default-insertion, and the handleSaiCreateStatus additions with rollback via deleteNetlinkChannel improve error handling. Previous review concerns (default m_oid init, m_table_name ownership) have been addressed in c470f11. LGTM.

banidoru

All reviewers approved. LGTM.

mssonicbld · 2026-03-18T21:30:18Z

/azp run

mssonicbld · 2026-03-18T21:30:18Z

/azp run

azure-pipelines · 2026-03-18T21:30:30Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2026-03-18T21:30:30Z

Azure Pipelines successfully started running 1 pipeline(s).

vmittal-msft · 2026-03-19T01:22:47Z

@Pterosaur please help with branch rebase and passing PR checkers

Pterosaur added 5 commits March 9, 2026 23:12

Pterosaur force-pushed the fix/hft-orchagent-bugs branch from 0e0a618 to b67cef0 Compare March 9, 2026 23:12

Pterosaur changed the title ~~hftelorch: fix multiple bugs in high frequency telemetry orchagent~~ hftelorch: minor improvements and cleanups for high frequency telemetry Mar 9, 2026

Pterosaur added 2 commits March 10, 2026 00:23

hftelprofile: use u32 for TAM counter subscription stat id

7efaa82

SAI_TAM_COUNTER_SUBSCRIPTION_ATTR_STAT_ID is defined as sai_uint32_t in the SAI metadata. Use attr.value.u32 instead of attr.value.oid when creating TAM counter subscription objects. Signed-off-by: Ze Gan <[email protected]>

banidoru reviewed Mar 10, 2026

View reviewed changes

orchagent/high_frequency_telemetry/counternameupdater.h Outdated Show resolved Hide resolved

orchagent/high_frequency_telemetry/hftelorch.cpp Outdated Show resolved Hide resolved

orchagent/high_frequency_telemetry/hftelorch.cpp Show resolved Hide resolved

banidoru reviewed Mar 10, 2026

View reviewed changes

orchagent/high_frequency_telemetry/hftelorch.cpp Outdated Show resolved Hide resolved

banidoru approved these changes Mar 10, 2026

View reviewed changes

r12f added the Request for 202511 Branch label Mar 11, 2026

Pterosaur marked this pull request as ready for review March 12, 2026 13:42

Pterosaur requested a review from prsunny as a code owner March 12, 2026 13:42

Copilot AI review requested due to automatic review settings March 12, 2026 13:42

Copilot started reviewing on behalf of Pterosaur March 12, 2026 13:42 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

orchagent/high_frequency_telemetry/counternameupdater.cpp Show resolved Hide resolved

orchagent/high_frequency_telemetry/hftelorch.cpp Outdated Show resolved Hide resolved

Pterosaur added the Request for msft-202412 Branch label Mar 12, 2026

hftelorch: harden message ownership and partial create cleanup

c470f11

Signed-off-by: Ze Gan <[email protected]>

Pterosaur removed the Request for msft-202412 Branch label Mar 12, 2026

Pterosaur requested a review from r12f March 12, 2026 14:01

banidoru approved these changes Mar 12, 2026

View reviewed changes

Merge branch 'master' into fix/hft-orchagent-bugs

a9cdd81

banidoru approved these changes Mar 12, 2026

View reviewed changes

r12f approved these changes Mar 15, 2026

View reviewed changes

vmittal-msft added the Approved for 202511 Branch label Mar 16, 2026

Merge branch 'master' into fix/hft-orchagent-bugs

cf1b7d1

Conversation

Pterosaur commented Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Mar 9, 2026

Uh oh!

mssonicbld commented Mar 9, 2026

Uh oh!

azure-pipelines bot commented Mar 9, 2026

Uh oh!

azure-pipelines bot commented Mar 9, 2026

Uh oh!

mssonicbld commented Mar 10, 2026

Uh oh!

azure-pipelines bot commented Mar 10, 2026

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

banidoru left a comment

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Mar 18, 2026

Uh oh!

mssonicbld commented Mar 18, 2026

Uh oh!

azure-pipelines bot commented Mar 18, 2026

Uh oh!

azure-pipelines bot commented Mar 18, 2026

Uh oh!

vmittal-msft commented Mar 19, 2026

Uh oh!

Reviewers

Assignees

Pterosaur commented Mar 9, 2026 •

edited

Loading