fix(packetparser): Fix under reporting of TCP flags and packet metrics, improve scalability

[mmckeen]

Description

Previously packetparser in high dataAggregationLevel would report (mostly) every single packet since important flags were observed over the lifetime of the connection.

This changes that behavior to only observe the important flags on individual packets and report when necessary.

This will mean less packets are reported. However, it also adds back weighting for bytes, packets, and TCP flags so that metrics remain accurate versus before.

I also noticed the current docs for the TCP flags metrics are inaccurate, we only report a subset of the supported flags. Not sure if this is intentional, however supporting more flags will put more memory pressure on both conntrack as well as performance pressure on packet reporting. With sampling in place, this should be more than worth it but there may be repercussions for the performance of low dataAggregationLevel.

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

eBPF objects compile and load as expected.

main Branch

This Branch

Additional Notes

#1628 will be a follow-up to this to add additional sampling functionality.

---

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

[mmckeen]

@nddq @SRodi this is ready for review 🙇

[nddq]

@mmckeen sorry for the delay, I just got back from a break. I’ve gone through your proposed change a couple of times, and it looks solid to me. In fact, it addresses something we initially overlooked when conntrack was introduced. First, a few points to make sure we're aligned:

Previously packetparser in high dataAggregationLevel would report (mostly) every single packet since important flags were observed over the lifetime of the connection.

That’s correct. For any given packet, we currently:

• Report it if it contains a flag set we haven’t seen before for this specific connection
• Report it if a certain amount of time has passed since the last reported packet for this connection (default: 30 seconds; applies to both dataAggregation levels)
• Otherwise, we skip it

In a typical TCP connection, the reported events would likely look like:
SYN, SYN-ACK, ACK, PSH, PSH-ACK, (30 secs), PSH, PSH-ACK, ... FIN, FIN-ACK, FIN-ACK

As a result, we ignore all packets during those 30-second windows, which skews the reported packet, byte, and TCP flag counts from the actual values.

That said:

This changes that behavior to only observe the important flags on individual packets and report when necessary.

Could you clarify this part? From what I see, conntrack already behaves this way today — so I’m not sure this change introduces new behavior?

This will mean less packets are reported. However, it also adds back weighting for bytes, packets, and TCP flags so that metrics remain accurate versus before.

This is great — it addresses the gap I mentioned earlier around ignored packets. So in that sense, this feels more like a bug fix than a new feature 🙂

[mmckeen]

@mmckeen sorry for the delay, I just got back from a break. I’ve gone through your proposed change a couple of times, and it looks solid to me. In fact, it addresses something we initially overlooked when conntrack was introduced. First, a few points to make sure we're aligned:

Previously packetparser in high dataAggregationLevel would report (mostly) every single packet since important flags were observed over the lifetime of the connection.

That’s correct. For any given packet, we currently:

• Report it if it contains a flag set we haven’t seen before for this specific connection
• Report it if a certain amount of time has passed since the last reported packet for this connection (default: 30 seconds; applies to both dataAggregation levels)
• Otherwise, we skip it

In a typical TCP connection, the reported events would likely look like: SYN, SYN-ACK, ACK, PSH, PSH-ACK, (30 secs), PSH, PSH-ACK, ... FIN, FIN-ACK, FIN-ACK

As a result, we ignore all packets during those 30-second windows, which skews the reported packet, byte, and TCP flag counts from the actual values.

That said:

This changes that behavior to only observe the important flags on individual packets and report when necessary.

Could you clarify this part? From what I see, conntrack already behaves this way today — so I’m not sure this change introduces new behavior?

This will mean less packets are reported. However, it also adds back weighting for bytes, packets, and TCP flags so that metrics remain accurate versus before.

This is great — it addresses the gap I mentioned earlier around ignored packets. So in that sense, this feels more like a bug fix than a new feature 🙂

I think it's a bit of both a bug fix and a new feature.

This will also always report packets if we hit important flags like TCP_URG TCP_ECE but only for the packet with that flag and not for the rest of the connection.

But yes, I think overall this is more a bug fix and that is just a minor change to reflect expected functionality that the 30 second reporting window is respected for connections without new flags.

[mmckeen]

@nddq @SRodi any further thoughts on this?

[nddq]

Overall lgtm, beside a minor comment. May I suggest naming this PR to something along the lines of fix underreporting of TCP metrics due to conntrack reporting logic? @SRodi for another pair of eyes

[mmckeen]

@nddq added the remaining flags, appreciate one more review when you get the chance 🙇

[SRodi]

@mmckeen just a couple of very minor nit. - one question, would it be worth to add this to our documentation?

[mmckeen]

@mmckeen just a couple of very minor nit. - one question, would it be worth to add this to our documentation?

Probably, happy to follow up on that with a future PR 🙇

[SRodi]

LGTM @mmckeen - please update the branch prior merge