feat(packetparser): Allow sampling of packets by mmckeen · Pull Request #1767 · microsoft/retina

mmckeen · 2025-07-22T22:36:19Z

Description

This PR allows for optional sampling of packet reporting when in high data aggregation level for packetparser.

By default, all packets are reported but optionally 1 out of n packets are sampled by random chance with the exception of certain important control flags or when hitting the reporting interval.

This allows Retina to scale to high network volume environments at the trade-off of some reporting granularity.

The performance impact of this is mostly for workloads with lots of new connections, connections already tracked in the conntrack table rely on #1665 for scalability.

The behavior added in #1665 allows for accurate reporting of metrics despite sampling being in place.

Related Issue

#1760

Checklist

I have read the contributing documentation.
I signed and signed-off the commits (git commit -S -s ...). See this documentation on signing commits.
I have correctly attributed the author(s) of the code.
I have tested the changes locally.
I have followed the project's style guidelines.
I have updated the documentation, if necessary.
I have added tests, if applicable.

Screenshots (if applicable) or Testing Completed

Main

After the change (with default sampling rate of 1)

After the change (with sampling rate of 1000)

Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.

mmckeen · 2025-08-12T15:57:37Z

@nddq @SRodi any thoughts on this PR?

nddq · 2025-08-21T17:48:07Z

@mmckeen I will try to make a pass this week

nddq

I think we should revisit the syntax of DataSamplingRate. For example, since I know the code context, I understand that setting DataSamplingRate to 1 means every packet will be sampled. But for someone new to Retina, this behavior may not be obvious.

nddq · 2025-09-02T03:13:14Z

Also, I'm all for ditching the reporting period entirely if we can make packet sampling works as it is a lot more flexible than the currently hardcoding value that we are using for reporting period 🙂 cc @anubhabMajumdar @SRodi So what I'm thinking of is either we report a packet because it carries a new flag or because it was randomly sampled. Some more advanced behavior that I could think of is that we can monitor cpu usage and dynamically adjust the sampling rate and recompile the packet parser program accordingly.

mmckeen · 2025-09-02T20:12:16Z

Also, I'm all for ditching the reporting period entirely if we can make packet sampling works as it is a lot more flexible than the currently hardcoding value that we are using for reporting period 🙂 cc @anubhabMajumdar @SRodi So what I'm thinking of is either we report a packet because it carries a new flag or because it was randomly sampled. Some more advanced behavior that I could think of is that we can monitor cpu usage and dynamically adjust the sampling rate and recompile the packet parser program accordingly.

The reporting interval might still be valid to enable us to make sure we report eventually for every 5-tuple. For example, we wouldn't want to sample and then never report the counters for a particular connection.

The reporting interval being something smallish also ensures Prometheus rate interval calculations (per-minute for example) stay valid.

github-actions · 2025-10-09T00:23:57Z

This PR will be closed in 7 days due to inactivity.

mmckeen · 2025-10-21T18:33:42Z

Apologies, been a while for me to circle back on this.

I'll try and address the comments this week 🙇

mmckeen · 2025-10-22T22:55:16Z

@nddq updated this with docs, the sampling calculation still looks accurate to me but happy for feedback 🙇

nddq · 2025-10-23T18:57:18Z

@mmckeen thanks for the docs! After re-reading it, the sampling calculation makes more sense to me now (I thought by default, the sampled variable is set to false 😅). Will try to do another round of review soon.

github-actions · 2025-11-23T00:30:09Z

This PR will be closed in 7 days due to inactivity.

mmckeen · 2025-12-01T16:50:26Z

@nddq @SRodi any progress on this?

nddq · 2025-12-02T22:04:58Z

hey @mmckeen , I just merged some changes for conntrack, can you rebase your PR? Thanks!

mmckeen · 2025-12-02T22:52:29Z

hey @mmckeen , I just merged some changes for conntrack, can you rebase your PR? Thanks!

Done!

Signed-off-by: Matthew McKeen <matthew.mckeen@fastly.com>

nddq

lgtm, thanks!

# Description This PR fixes unit test failures in the `packetparser` plugin: - Mocks `metrics.ParsedPacketsCounter` in `TestReadDataPodLevelEnabled` to prevent a nil pointer dereference during test execution. - Updates the expected dynamic header content in `TestPacketParseGenerate` to match the actual generated output, which now includes `DATA_SAMPLING_RATE` and `BYPASS_LOOKUP_IP_OF_INTEREST` definitions. ## Related PRs This test was broken in the following PRs - #624 - #1767 ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/Contributing/overview). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. ## Screenshots (if applicable) or Testing Completed <img width="1624" height="1343" alt="image" src="https://github.com/user-attachments/assets/9549ed15-7c62-4537-a806-2d2368d3fc66" /> ## Additional Notes We have a bigger issue which is CI is not showing a failure when single test fails, we will look into that and create another PR to fix that. See this example (FAIL not caught) - reference #1688 https://github.com/microsoft/retina/actions/runs/20115412850/job/57723528708#step:4:3145 --- Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more information on how to contribute to this project.

# Description This PR fixes unit test failures in the `packetparser` plugin: - Mocks `metrics.ParsedPacketsCounter` in `TestReadDataPodLevelEnabled` to prevent a nil pointer dereference during test execution. - Updates the expected dynamic header content in `TestPacketParseGenerate` to match the actual generated output, which now includes `DATA_SAMPLING_RATE` and `BYPASS_LOOKUP_IP_OF_INTEREST` definitions. ## Related PRs This test was broken in the following PRs - microsoft#624 - microsoft#1767 ## Checklist - [x] I have read the [contributing documentation](https://retina.sh/docs/Contributing/overview). - [x] I signed and signed-off the commits (`git commit -S -s ...`). See [this documentation](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification) on signing commits. - [x] I have correctly attributed the author(s) of the code. - [x] I have tested the changes locally. - [x] I have followed the project's style guidelines. - [x] I have updated the documentation, if necessary. - [x] I have added tests, if applicable. ## Screenshots (if applicable) or Testing Completed <img width="1624" height="1343" alt="image" src="https://github.com/user-attachments/assets/9549ed15-7c62-4537-a806-2d2368d3fc66" /> ## Additional Notes We have a bigger issue which is CI is not showing a failure when single test fails, we will look into that and create another PR to fix that. See this example (FAIL not caught) - reference microsoft#1688 https://github.com/microsoft/retina/actions/runs/20115412850/job/57723528708#step:4:3145 --- Please refer to the [CONTRIBUTING.md](../CONTRIBUTING.md) file for more information on how to contribute to this project.

mmckeen force-pushed the packetParserSampling branch 3 times, most recently from ebac9e3 to b2a5ab5 Compare July 22, 2025 23:08

mmckeen marked this pull request as ready for review July 23, 2025 00:05

mmckeen requested a review from a team as a code owner July 23, 2025 00:05

mmckeen requested review from karina-ranadive and mainred July 23, 2025 00:05

agrawaliti mentioned this pull request Jul 31, 2025

Retina-agent high CPU usage #1760

Closed

nddq requested review from SRodi and nddq and removed request for karina-ranadive and mainred August 1, 2025 03:40

nddq added the type/enhancement New feature or request label Aug 1, 2025

nddq requested changes Sep 2, 2025

View reviewed changes

Comment thread pkg/config/config.go

Comment thread pkg/plugin/packetparser/_cprog/packetparser.c

Comment thread pkg/plugin/packetparser/_cprog/packetparser.c

github-actions Bot added the meta/waiting-for-author Blocked and waiting on the author label Oct 9, 2025

nddq removed the meta/waiting-for-author Blocked and waiting on the author label Oct 11, 2025

mmckeen force-pushed the packetParserSampling branch 2 times, most recently from 5ddf47d to 49ed563 Compare October 22, 2025 22:48

mmckeen requested a review from nddq October 22, 2025 22:54

github-actions Bot added the meta/waiting-for-author Blocked and waiting on the author label Nov 23, 2025

nddq removed the meta/waiting-for-author Blocked and waiting on the author label Nov 24, 2025

mmckeen force-pushed the packetParserSampling branch 2 times, most recently from 49ed563 to a7e95f4 Compare December 2, 2025 22:51

feat(packetparser): Allow sampling of packets

12c6e02

Signed-off-by: Matthew McKeen <matthew.mckeen@fastly.com>

mmckeen force-pushed the packetParserSampling branch from a7e95f4 to 12c6e02 Compare December 2, 2025 23:01

nddq approved these changes Dec 3, 2025

View reviewed changes

nddq added this pull request to the merge queue Dec 3, 2025

Merged via the queue into microsoft:main with commit f4a818e Dec 3, 2025
31 checks passed

SRodi mentioned this pull request Dec 11, 2025

fix(tests): Fix packetparser tests #1977

Merged

7 tasks

Conversation

mmckeen commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Checklist

Screenshots (if applicable) or Testing Completed

Main

After the change (with default sampling rate of 1)

After the change (with sampling rate of 1000)

Uh oh!

mmckeen commented Aug 12, 2025

Uh oh!

nddq commented Aug 21, 2025

Uh oh!

nddq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nddq commented Sep 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmckeen commented Sep 2, 2025

Uh oh!

github-actions Bot commented Oct 9, 2025

Uh oh!

mmckeen commented Oct 21, 2025

Uh oh!

mmckeen commented Oct 22, 2025

Uh oh!

nddq commented Oct 23, 2025

Uh oh!

github-actions Bot commented Nov 23, 2025

Uh oh!

mmckeen commented Dec 1, 2025

Uh oh!

nddq commented Dec 2, 2025

Uh oh!

mmckeen commented Dec 2, 2025

Uh oh!

nddq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mmckeen commented Jul 22, 2025 •

edited

Loading

nddq commented Sep 2, 2025 •

edited

Loading