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

Assorted small fixes for dogstatsd UDS listener. #33582

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

vickenty
Copy link
Contributor

What does this PR do?

Fix assorted small issues in dogstatsd uds listener.

Motivation

Improve error reporting and fix potential issues with partial reads from stream socket.

Describe how you validated your changes

New test added to cover the correctness issue with partial read.

Possible Drawbacks / Trade-offs

Additional Notes

Produce more verbose error message when EOF happens after reading some
bytes, but not all, as it indictates connection sync error.

Also log all other errors and treat them as fatal.
SCM_CREDENTIALS are always written in full from the start for each read, even if the
previous was partial (which shouldn't happen in our case).
Read returns number of bytes read by that one particular call, not the
final position in the buffer. Reusing only the previous return value
will overwrite previously received data.

If reads sufficiently small, number of bytes read may never be equal
to expectedPacketLength, causing the loop to never complete.
@vickenty vickenty added changelog/no-changelog qa/done QA done before merge and regressions are covered by tests labels Jan 30, 2025
@github-actions github-actions bot added the medium review PR review might take time label Jan 30, 2025
@vickenty vickenty changed the title Vickenty/fdl Assorted small fixes for dogstatsd UDS listener. Jan 30, 2025
@agent-platform-auto-pr
Copy link
Contributor

agent-platform-auto-pr bot commented Jan 30, 2025

Uncompressed package size comparison

Comparison with ancestor 27786659d1ae84ee28f641900f4dc4a424ba4d1e

Diff per package
package diff status size ancestor threshold
datadog-dogstatsd-x86_64-rpm 0.00MB 58.95MB 58.95MB 0.50MB
datadog-dogstatsd-x86_64-suse 0.00MB 58.95MB 58.95MB 0.50MB
datadog-agent-amd64-deb 0.00MB 928.61MB 928.60MB 0.50MB
datadog-dogstatsd-amd64-deb 0.00MB 58.87MB 58.87MB 0.50MB
datadog-agent-x86_64-rpm 0.00MB 938.35MB 938.34MB 0.50MB
datadog-agent-x86_64-suse 0.00MB 938.35MB 938.34MB 0.50MB
datadog-iot-agent-amd64-deb 0.00MB 93.92MB 93.91MB 0.50MB
datadog-iot-agent-aarch64-rpm 0.00MB 90.04MB 90.04MB 0.50MB
datadog-iot-agent-x86_64-rpm 0.00MB 93.98MB 93.98MB 0.50MB
datadog-iot-agent-x86_64-suse 0.00MB 93.98MB 93.98MB 0.50MB
datadog-agent-arm64-deb 0.00MB 915.81MB 915.81MB 0.50MB
datadog-dogstatsd-arm64-deb 0.00MB 56.38MB 56.38MB 0.50MB
datadog-heroku-agent-amd64-deb 0.00MB 477.83MB 477.83MB 0.50MB
datadog-iot-agent-arm64-deb 0.00MB 89.97MB 89.97MB 0.50MB
datadog-agent-aarch64-rpm -0.00MB 925.53MB 925.53MB 0.50MB

Decision

✅ Passed

@agent-platform-auto-pr
Copy link
Contributor

agent-platform-auto-pr bot commented Jan 30, 2025

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv aws.create-vm --pipeline-id=54456568 --os-family=ubuntu

Note: This applies to commit 78e9185

@vickenty vickenty marked this pull request as ready for review January 30, 2025 15:53
@vickenty vickenty requested a review from a team as a code owner January 30, 2025 15:53
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

LGTM.

Left one non-blocking question.


for err := range sync {
require.NoError(t, err)
time.Sleep(10 * time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

Just a small nit: what's the reason to be adding a sleep here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/no-changelog medium review PR review might take time qa/done QA done before merge and regressions are covered by tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants