Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send a final WireMeasurement after byte limit reached #30

Merged
merged 3 commits into from
Jan 4, 2024

Conversation

stephen-soltesz
Copy link
Contributor

@stephen-soltesz stephen-soltesz commented Jan 3, 2024

This change adds logic to ensure that a final WireMeasurement is sent after a byte limit is reached for downloads or uploads.

The changes from #29 evolved from behaving similarly to m-lab/ndt-server#390 -- where connections are closed after a measurer polling event -- to one where connections are closed after stricter byte limits are sent.

The original approach delayed connection close and could send significantly more data than a user requested / expected. The new approach does not guarantee that the client will receive any polling measurements because the connection may close before any are sent.

This change ensures that at least one measurement is sent after closing the client connection.

NOTE: the message size is not accounted for in the over all byte limit, and for small byte limits the total bytes sent may be larger than requested, but in general the size of a measurement message is considered negligible. This change also expects that a client using a byte limit will read all messages before closing the client end of the connection.


This change is Reviewable

@coveralls
Copy link

coveralls commented Jan 3, 2024

Pull Request Test Coverage Report for Build 7403242148

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 69.751%

Totals Coverage Status
Change from base Build 7399078411: 0.3%
Covered Lines: 867
Relevant Lines: 1243

💛 - Coveralls

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's not particularly easy to get the raw message at the moment, I have verified that the "Application BytesSent" matches the requested bytes sent.

With parameters:

bytes=15000&streams=1&cc=bbr
{
  "CC": "bbr",
  "UUID": "msak-r8l8f_1703027492_0000000000100073",
  "LocalAddr": "4.14.3.49:443",
  "RemoteAddr": "67.83.5.53:63681",
  "Application": {
    "BytesSent": 15000
  },
  "Network": {
    "BytesSent": 15750
  },
  "TCPInfo": {
    "BytesSent": 13312,
}

I don't understand why TCPInfo is lower. That seems easy to reproduce. I don't know if that's expected (e.g. bytes are still in the network buffer and haven't been counted yet?).

Reviewable status: 0 of 1 approvals obtained (waiting on @robertodauria)

Copy link
Contributor

@robertodauria robertodauria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: I have tested this by writing the measurements in/out on both the server and the client, it appears to work as expected.

NOTE: the message size is not accounted for in the over all byte limit, and for small byte limits the total bytes sent may be larger than requested, but in general the size of a measurement message is considered negligible. This change also expects that a client using a byte limit will read all messages before closing the client end of the connection.

Do you mean the last Measurement size isn't accounted for since the snapshot is taken before sending it? If so, agreed. In the general case, the size of a JSON Measurement is accounted for (in sendWireMeasurement) but I see no way of reporting the last one to the client.

I don't understand why TCPInfo is lower. That seems easy to reproduce. I don't know if that's expected (e.g. bytes are still in the network buffer and haven't been counted yet?).

Possibly. Network BytesSent/Received are offset by the initial values of the byte counters when the Measurer is instantiated, so if anything they should be a bit lower. These counters are updated after conn.Write and conn.Read return, but the socket is non-blocking, so I guess it's possible that Network.BytesSent is updated just before TCPInfo.BytesSent.

Reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

Copy link
Contributor Author

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I meant the last Measurement size.

Regarding Network bytes / TCPInfo bytes -- the former is maintained by the application, the latter by the Kernel. And, I suspect our instincts are correct that the kernel does not increment TCPInfo metrics until they are actually "Bytes Sent". And since this final message is sampled immediately after writing the final message it could easily be in the kernel buffer still.

Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@stephen-soltesz stephen-soltesz merged commit af9f6c4 into main Jan 4, 2024
8 checks passed
@stephen-soltesz stephen-soltesz deleted the sandbox-soltesz-final-wiremessage branch January 4, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants