Add nanosecond precision to TCP reassembly #1591

seladb · 2024-09-26T09:34:28Z

Should address this issue: #1189

codecov · 2024-09-26T09:51:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.14%. Comparing base (98ecd59) to head (91f6b39).
Report is 1 commits behind head on dev.

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1591      +/-   ##
==========================================
+ Coverage   83.12%   83.14%   +0.02%     
==========================================
  Files         276      276              
  Lines       46914    46964      +50     
  Branches     9346     9315      -31     
==========================================
+ Hits        38998    39050      +52     
+ Misses       7061     7055       -6     
- Partials      855      859       +4

Flag	Coverage Δ
fedora40	`74.76% <100.00%> (+0.02%)`	⬆️
macos-12	`81.13% <100.00%> (+0.02%)`	⬆️
macos-13	`80.56% <100.00%> (+0.02%)`	⬆️
macos-14	`80.48% <100.00%> (+0.02%)`	⬆️
mingw32	`70.65% <81.25%> (+0.01%)`	⬆️
mingw64	`70.60% <81.25%> (-0.01%)`	⬇️
npcap	`85.02% <100.00%> (+<0.01%)`	⬆️
rhel94	`74.57% <100.00%> (-0.02%)`	⬇️
ubuntu2004	`57.91% <31.81%> (-0.03%)`	⬇️
ubuntu2004-zstd	`58.04% <31.81%> (-0.03%)`	⬇️
ubuntu2204	`74.50% <100.00%> (+0.01%)`	⬆️
ubuntu2204-icpx	`58.71% <100.00%> (+0.01%)`	⬆️
ubuntu2404	`74.75% <100.00%> (+0.01%)`	⬆️
unittest	`83.14% <100.00%> (+0.02%)`	⬆️
windows-2019	`85.06% <100.00%> (+<0.01%)`	⬆️
windows-2022	`85.06% <100.00%> (+<0.01%)`	⬆️
winpcap	`85.03% <100.00%> (+0.01%)`	⬆️
xdp	`49.58% <0.00%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Dimi1010 · 2024-09-26T22:42:39Z

Packet++/header/TcpReassembly.h

-		{
-			endTime = endTimeValue;
-		}
+		void setEndTime(const std::chrono::time_point<std::chrono::system_clock>& endTimeValue);


Why are the timepoints locked to system_clock? Generally I would prefer steady_clock for being monotonic, or high_resolution_clock for being the clock with the highest resolution on the system. Ideally, it should be templated, but that might be more trouble than its worth.

What we need here is a data structure that holds a timestamp in nanosecond precision. The timestamp itself comes from the packet:

PcapPlusPlus/Packet++/src/TcpReassembly.cpp

Line 142 in 634a786

auto currTime = timespecToTimePoint(tcpData.getRawPacket()->getPacketTimeStamp());

Does it matter if it's system_clock, steady_clock or high_resolution_clock? 🤔

Not sure tbh, but I think there are a some methods of timepoint that depend on the passed clock type. It probably won't really affect the current usage, as nowhere is Clock::now() called. so idk.

I tested it, apparently system_clock doesn't have nanosecond precision on all platforms, however, high_resolution_clock does so I think it's safe to use

clementperon · 2024-09-28T09:48:55Z

Packet++/header/TcpReassembly.h

 		timeval endTime;
+		/** Start timestamp of the connection with nanosecond precision */
+		std::chrono::time_point<std::chrono::system_clock> startTimePrecise;


It's not guarantee that std::chrono::time_point<std::chrono::system_clock> have nano seconds resolution.
It depends on Clock::duration. On macOS, system_clock::duration is microseconds.

IMO i would explicit which clock::duration we want to have or change all the timeval to timespec.

Tbh, using timespec might be simpler, as the timepoints in the chrono library are more specialized to be used with a clock, and we get the time externally anyway so we don't really have one.

Please see: #1591 (comment)

I meant we whould explicit the duration like this:

using time_point_ns = std::chrono::time_point<std::chrono::high_resolution_clock, std::chrono::nanoseconds>; time_point_ns startTimePrecise; time_point_ns endTimePrecise;

This will also work for std::chrono::system_clock

Why do we need to be explicit? If we do:

std::chrono::time_point<std::chrono::high_resolution_clock>

Then the user can decide which precision they want, for example, if they want nano sec they can do the following:

std::chrono::duration_cast<std::chrono::nanoseconds>(stats.begin()>second.connData.startTimePrecise.time_since_epoch())

But they can also get any other precision

- Add some tests

tigercosmos · 2024-10-02T08:28:06Z

Tests/Pcap++Test/Tests/TcpReassemblyTests.cpp

-	PTF_ASSERT_EQUAL(stats.begin()->second.connData.endTime.tv_usec, 0);
+	PTF_ASSERT_EQUAL(std::chrono::duration_cast<std::chrono::nanoseconds>(
+	                     stats.begin()->second.connData.startTimePrecise.time_since_epoch())
+	                     .count(),


Can we have a better format here?

so just ignore clang-format? 🤔

I think you can ignore it here as the format seems not perfect.

Done in this commit b083640, please let me know if the format looks ok now

Dimi1010

A couple of comments, but LGTM otherwise.

Dimi1010 · 2024-10-04T08:30:36Z

Packet++/src/TcpReassembly.cpp

+		auto duration = in.time_since_epoch();
+
+		auto seconds = std::chrono::duration_cast<std::chrono::seconds>(duration);
+		auto nanoseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(duration) % 1000000000;


nit: Could possibly be done without the magic number by casting the seconds to nanosecond precision and substracting that from the main duration to get the remaining nanoseconds, but I guess this works too.

Ture, I think Dimi's method will be more efficient.

Ture, I think Dimi's method will be more efficient.

Looked through it in godbolt.org. It actually does not matter under optimizations. Both compile down to mostly the same assembly.

Amazing compiler

I agree! Fixed in 91f6b39

Dimi1010 · 2024-10-04T08:37:59Z

Packet++/src/TcpReassembly.cpp

+
+		struct timeval out;
+		out.tv_sec = seconds.count();
+		out.tv_usec = nanoseconds.count() / 1000;


Why do we calculate the nanosecond precision here if we are just going to convert them to microseconds? As far as I see, the nanosecond precision duration object is not used anywhere else in this function.

Good catch! Fixed in 91f6b39

Switch TCP reassembly timestamp to nanosecond precision

634a786

Dimi1010 reviewed Sep 26, 2024

View reviewed changes

clementperon reviewed Sep 28, 2024

View reviewed changes

seladb added 4 commits September 28, 2024 02:49

- Change to high_resolution_clock

c609925

- Add some tests

Add tests

e5027c4

fmt

4a846a5

Fix end-of-file

eff26f2

seladb marked this pull request as ready for review October 1, 2024 08:43

Merge branch 'dev' into tcp-reassembly-nano-precision

fdb7f19

seladb requested review from clementperon, Dimi1010, tigercosmos and egecetin October 1, 2024 08:44

seladb linked an issue Oct 1, 2024 that may be closed by this pull request

Question regarding the drop of nanosecond precision timestamp #1189

Closed

tigercosmos reviewed Oct 2, 2024

View reviewed changes

Ignore formatting

b083640

tigercosmos approved these changes Oct 3, 2024

View reviewed changes

Dimi1010 approved these changes Oct 4, 2024

View reviewed changes

seladb added 2 commits October 8, 2024 01:00

Merge branch 'dev' into tcp-reassembly-nano-precision

3c72ddb

Address PR comments

91f6b39

seladb merged commit 9f31f71 into dev Oct 8, 2024
40 checks passed

seladb deleted the tcp-reassembly-nano-precision branch October 8, 2024 09:28

seladb mentioned this pull request Oct 8, 2024

Question regarding the drop of nanosecond precision timestamp #1189

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nanosecond precision to TCP reassembly #1591

Add nanosecond precision to TCP reassembly #1591

seladb commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

Dimi1010 Sep 26, 2024

seladb Sep 27, 2024

Dimi1010 Sep 27, 2024 •

edited

Loading

seladb Oct 1, 2024

clementperon Sep 28, 2024 •

edited

Loading

Dimi1010 Sep 28, 2024 •

edited

Loading

seladb Oct 1, 2024

clementperon Oct 1, 2024

seladb Oct 2, 2024

tigercosmos Oct 2, 2024

seladb Oct 2, 2024

tigercosmos Oct 2, 2024

seladb Oct 3, 2024

tigercosmos Oct 3, 2024

Dimi1010 left a comment

Dimi1010 Oct 4, 2024 •

edited

Loading

tigercosmos Oct 4, 2024

Dimi1010 Oct 4, 2024

tigercosmos Oct 5, 2024

seladb Oct 8, 2024

Dimi1010 Oct 4, 2024

seladb Oct 8, 2024

Add nanosecond precision to TCP reassembly #1591

Add nanosecond precision to TCP reassembly #1591

Conversation

seladb commented Sep 26, 2024 • edited Loading

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 Sep 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clementperon Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Dimi1010 Sep 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dimi1010 left a comment

Choose a reason for hiding this comment

Dimi1010 Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seladb commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

Dimi1010 Sep 27, 2024 •

edited

Loading

clementperon Sep 28, 2024 •

edited

Loading

Dimi1010 Sep 28, 2024 •

edited

Loading

Dimi1010 Oct 4, 2024 •

edited

Loading