fix(node-agent): fix ContainerProfile size accounting in ReportSyscall and ReportNetworkEvent

[aryanghai12]

Overview

• Current Behavior: The MaxTsProfileSize pre-send threshold undercounted ContainerProfile byte sizes due to mixed accumulator units. ReportSyscall added mapset.Set.Append's return value (0 or 1) rather than the actual syscall byte size. ReportNetworkEvent only measured the raw NetworkEvent struct, ignoring post-serialization field expansions (DNS, PodSelector, NamespaceSelector, hashes) added later by createNetworkNeighbor. As a result, profiles grew past storage limits instead of flushing early, triggering HTTP 413 payload errors.
• Future Behavior: ReportSyscall adds size.Of(syscall) only when a syscall is newly appended. ReportNetworkEvent applies a conservative 256-byte networkNeighborExpansionEstimate surcharge on top of size.Of(networkEvent) to account for deferred serialization expansions. Profiles now flush accurately before hitting storage caps.

Additional Information

• Syscall Accounting: Fixed the unit mismatch where element-count deltas were being added directly to the byte accumulator.
• Network Neighbor Expansion: Because DNS resolution and Service selector lookups are deferred to serialization, exact expansion sizes cannot be calculated at report time; the fixed 256-byte surcharge provides a safe, conservative buffer.

How to Test

Run the size accounting unit tests:

go test -v ./... -run 'TestReportSyscallSizeAccounting|TestReportNetworkEventSizeAccounting'

Examples/Screenshots

N/A (backend byte accounting fix)

Related issues/PRs:

• Resolved containerprofilemanager's pre-send size estimator mixes byte-size and element-count units #870

Checklist before requesting a review

• My code follows the style guidelines of this project
• I have commented on my code, particularly in hard-to-understand areas
• I have performed a self-review of my code
• If it is a core feature, I have added thorough tests.
• New and existing unit tests pass locally with my changes

Please open the PR against the dev branch (Unless the PR contains only documentation changes)

Summary by CodeRabbit

• Bug Fixes

  • Improved container profile size estimates for network events and syscall reporting.
  • Duplicate syscalls and repeated network reports no longer inflate size calculations.

• Tests

  • Added coverage to verify accurate accounting for unique syscalls, network-neighbor data, and duplicate reports.

• Chores

  • Updated ignored development artifacts.

[coderabbitai]

Warning

Review limit reached

@aryanghai12, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ff95c69a-6937-4171-8ca4-0c9be9f4ab2d

📥 Commits

Reviewing files that changed from the base of the PR and between c576f98 and fe073b2.

📒 Files selected for processing (3)

• .gitignore
• pkg/containerprofilemanager/v1/event_reporting.go
• pkg/containerprofilemanager/v1/event_reporting_test.go

📝 Walkthrough

Walkthrough

The container profile estimator now uses serialized-size accounting for unique syscalls and network-neighbor expansion. Tests cover the updated estimates. The repository also ignores two local artifacts.

Changes

Container profile size accounting

Layer / File(s)	Summary
Serialized event estimates pkg/containerprofilemanager/v1/event_reporting.go, pkg/containerprofilemanager/v1/event_reporting_test.go	Network events include a 256-byte neighbor expansion estimate. Duplicate syscalls add no size. New syscalls add their serialized string size. Tests cover both behaviors.

Repository ignore rules

Layer / File(s)	Summary
Ignore local artifacts .gitignore	The ignore list includes tracers.tar and LFX_AGENT_SANDBOX_PREP.md.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The .gitignore updates for tracers.tar and LFX_AGENT_SANDBOX_PREP.md are unrelated to issue #870.	Remove the unrelated .gitignore entries or move them to a separate pull request.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes satisfy issue #870 by using serialized syscall byte sizes and estimating network-neighbor expansion before sending.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the ContainerProfile size-accounting fixes in both ReportSyscall and ReportNetworkEvent.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@pkg/containerprofilemanager/v1/event_reporting.go`:
- Around line 22-29: The fixed networkNeighborExpansionEstimate in event
reporting undercounts deferred NetworkNeighbor data. Replace it with an exact
pre-threshold size calculation or a documented maximum covering the identifier,
field encoding, DNS name, and selector payload; update the threshold accounting
around the relevant reporting function and add coverage for the maximum
supported DNS name and selector payload so requests remain within
MaxTsProfileSize.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ce72bc5c-6c86-4a53-a6ff-4425790dd696

📥 Commits

Reviewing files that changed from the base of the PR and between 5b5ff1c and c576f98.

📒 Files selected for processing (3)

• .gitignore
• pkg/containerprofilemanager/v1/event_reporting.go
• pkg/containerprofilemanager/v1/event_reporting_test.go

[matthyx]

Thanks for taking this on — the ReportSyscall half is exactly right, and the test coverage is genuinely good. One blocker on the network half before this can go in.

Blocker: the expansion estimate over-counts by ~6–20x, which trades the 413 undercount for constant premature splitting

networkNeighborExpansionEstimate evaluates to 4571 bytes. Component breakdown (measured on b9ad48c):

component	bytes
identifier	80
Type	24
Ports	85
DNS + DNSNames	562
NamespaceSelector	164
PodSelector	3656
total	4571

Measured against what createNetworkNeighbor actually produces:

case	estimate (size.Of(event) + surcharge)	actual size.Of(*neighbor)	ratio
external IP, no DNS	4726	242	19.5x
pod destination, 6 typical Helm labels	4930	767	6.4x

With the default maxTsProfileSize: 2Mi, that means a split fires after ~443 unique network events, whose real serialized weight is ~130–350 KB — roughly 6–15% of the intended threshold. Network neighbors are usually the dominant part of a profile, so in practice this makes ProfileRequiresSplit the normal path rather than the safety net #870 asks for ("Once this is fixed, the splitting mechanism added in #866 should become a rare safety net rather than something regularly exercised in the field"). Under-flushing became over-flushing; both miss the target.

The root cause is that the surcharge is unconditional, while createNetworkNeighbor takes exactly one branch per Destination.Kind — the doc comment says as much, but the code still sums all branches. And critically, most of the budget isn't actually unknown at report time:

• PodSelector for EndpointKindPod is already counted. It comes from filterLabels(networkEvent.GetDestinationPodLabels()), which parses Destination.PodLabels — a plain string field on the NetworkEvent that size.Of(networkEvent) already includes, and filterLabels only ever removes entries. So the 3656 bytes are double-counted on the pod branch and pure fiction on the raw/DNS branch (where PodSelector is nil).
• NamespaceSelector is exactly computable: the namespaces are both known at report time.
• Ports is exactly computable from Port/Protocol.
• Only two things are genuinely deferred: the DNS name (raw branch only) and the Service selector (EndpointKindService only).

Suggested shape — switch on event.Destination.Kind and add only that branch's cost:

// fixed for every neighbor
est := neighborFixedOverhead // identifier + Type + Ports

switch networkEvent.Destination.Kind {
case EndpointKindPod:
    // labels already inside size.Of(networkEvent); only the selector wrappers are new
    est += size.Of(namespaceSelectorFor(networkEvent.Destination.Namespace, namespace))
case EndpointKindService:
    est += serviceSelectorBudget // genuinely deferred, but a Service selector is a handful of labels
default:
    est += maxDNSNameBudget // 253*2, raw branch only
}

That lands typical events in the few-hundred-bytes range — still conservative, but without turning every profile into a split. Keeping the existing tests as upper-bound assertions works fine with this shape.

Non-blocking

• .gitignore: LFX_AGENT_SANDBOX_PREP.md is unrelated to this fix (CodeRabbit's out-of-scope check flags it too) and looks like a personal working file — that belongs in your global gitignore or .git/info/exclude. Please drop it here. While you're in the file, it'd be nice to end it with a newline.
• PR description is stale: it still describes a "conservative 256-byte networkNeighborExpansionEstimate", which was c576f98; b9ad48c replaced it with the computed 4571-byte value. Worth updating before merge so the commit history reads correctly.
• Other accumulators still over-count (out of scope for this PR, but #870 asks for all accumulators to be consistent, so worth a follow-up): ReportFileExec adds size.Of(exec) unconditionally even though data.execs.Set overwrites the same execIdentifier key, so a repeatedly-exec'd binary inflates the estimate without bound. ReportFileOpen re-adds size.Of(path) when only new flags are appended to an existing path. Both push the same direction as the blocker above.
• size.Of measures in-memory Go size, not the JSON payload storage rejects with a 413. The new tests assert estimate >= size.Of(*neighbor), which doesn't directly bound the wire size. That's the pre-existing convention in this file so I'm not asking you to change it, but the estimator's relationship to the actual HTTP cap stays approximate.

Verified locally

go build ./... and go test ./pkg/containerprofilemanager/... both pass on b9ad48c; the numbers above come from instrumenting that build. Happy to re-review as soon as the branch-aware sizing is in.

[matthyx]

Blocker resolved — this is the right shape now. Approving.

fe073b2 replaces the flat surcharge with networkNeighborIncrement(data, networkEvent), charging only the branch createNetworkNeighbor will actually take. Re-measured on this commit:

constant	bytes
neighborFixedOverhead	104
maxDNSNameEstimate	562
maxServiceSelectorEstimate	1875

case	estimate	actual size.Of(*neighbor)	ratio (was)
external IP, DNS unresolved	983	242	4.06x (19.5x)
external IP, 253-byte DNS name	983	535	1.84x
pod dest, 6 Helm labels, cross-ns	912	767	1.19x (6.4x)
pod dest, 6 Helm labels, same ns	802	682	1.18x
pod dest, 30 long labels	4152	4007	1.04x

The upper-bound property still holds in every case I tried — no undercounts — and events-until-split at the default maxTsProfileSize: 2Mi goes from ~443 back to ~2133 for the external-IP case. The residual 4x on unresolved DNS is inherent (whether ResolveIPAddress will hit isn't knowable at report time) and the comment says so; charging the RFC 1035 bound there is the right call.

The PodSelector handling is better than what I suggested — subtracting size.Of(Destination.PodLabels) and charging only the map-wrapper delta (clamped at zero) is exact rather than budgeted, and it holds up: on the 30-label stress case the estimate lands 145 bytes over a 4007-byte neighbor.

Things I checked before approving:

• The two namespaces agree. networkNeighborIncrement reads data.watchedContainerData.Namespace, which is populated from container.K8s.Namespace (pkg/containerwatcher/v2/containercallback.go:117) — the same value monitoring.go:205-206 later passes to createNetworkNeighbor. So the estimator and the serializer make the same getNamespaceMatchLabels call, not two different ones.
• No new race. watchedContainerData is read under entry.mu inside withContainer, the same lock lifecycle.go:154 writes it under. go test -race ./pkg/containerprofilemanager/v1/ is clean.
• Nil watchedContainerData errs conservative. Events reported before shared data is ready see sourceNamespace == "", which over-charges the NamespaceSelector rather than under-charging it.
• Cost is fine. The increment is ~2.5µs / 42 allocs more than the old constant on the pod branch, but it sits after the data.networks.Contains early return, so it's paid once per unique neighbor tuple, not per packet. Negligible against a profile's lifetime.
• .gitignore now nets out to just adding the missing trailing newline. Thanks.
• go build ./..., go vet, gofmt -l, and go test ./pkg/containerprofilemanager/... all clean on fe073b2.

Two things to tidy, neither blocking the approval:

1. Please update the PR description before merging — it still describes "a conservative 256-byte networkNeighborExpansionEstimate", which is two commits stale and describes a symbol that no longer exists. If this lands as a squash merge that text becomes the commit body.
2. Optional test gaps: nothing exercises the EndpointKindService branch (maxServiceSelectorEstimate is the one budget left that's a genuine guess) or the watchedContainerData == nil path. Also a theoretical 164-byte undercount if a EndpointKindPod/Service event ever arrives with an empty Destination.Namespace — report time sees "" == "" and skips the NamespaceSelector while serialization would add it. I don't think that's reachable in practice, so purely FYI.

The follow-ups I mentioned last time on ReportFileExec (unconditional size.Of(exec) despite execs.Set overwriting the same key) and ReportFileOpen (re-adding size.Of(path) for flag-only updates) are still open, but they're #870's remaining scope rather than this PR's — worth a separate issue.