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

Restore Dogstatsd exporter #10

Merged
merged 4 commits into from
Apr 14, 2020
Merged

Restore Dogstatsd exporter #10

merged 4 commits into from
Apr 14, 2020

Conversation

jmacd
Copy link
Contributor

@jmacd jmacd commented Apr 14, 2020

This imports the code that was removed in open-telemetry/opentelemetry-go#591 without significant changes.

There was a report that the default MaxPacketSize is too large, which I was able to reproduce on my development machine. I've set it to 9216, which is Jumbo ethernet size. Another good default would be 1500. (This is configurable.)

@jmacd
Copy link
Contributor Author

jmacd commented Apr 14, 2020

@chrisleavoy

@jmacd
Copy link
Contributor Author

jmacd commented Apr 14, 2020

I fixed the known problem, but there is still a defect where if the MaxPacketSize is too large, data won't be sent. I tested on an OS X machine that the call to Write() returns 0 bytes written when the packet is too large. So the only way to avoid MTU-too-small errors is to dynamically adjust the packet size when we see the "message too long" error. I'll leave that up for discussion and/or another PR.

@zlozano
Copy link

zlozano commented Apr 14, 2020

Does the default packet size take into account the UDP header size + max IP header size? I only raise this since the Datadog client leaves room in its max buffer size (1500 - 8 - 60)

https://github.com/DataDog/datadog-go/blob/master/statsd/statsd.go#L42

@zlozano
Copy link

zlozano commented Apr 14, 2020

Also, it seems like the safer default would be the more conservative option of 1500 (minus room for header bytes). This seems to be the more widely accepted default configuration [1]

[1] https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/network_mtu.html

@kconwayinvision
Copy link

I fixed the known problem, but there is still a defect where if the MaxPacketSize is too large, data won't be sent. I tested on an OS X machine that the call to Write() returns 0 bytes written when the packet is too large. So the only way to avoid MTU-too-small errors is to dynamically adjust the packet size when we see the "message too long" error. I'll leave that up for discussion and/or another PR.

Getting an error back from a UDP write is highly depending on the system and system configuration. For example, Ubunut/Debian builds have a max UDP packet size of 65535 bytes whereas OSX uses 9216 bytes. It's less of an error that the packet is too big for the network and more of an error that the network stack on that machine considers the packet too large. It's much more likely that the number of bytes being written is will be considered valid from the perspective of the network stack and silently dropped by the networking infrastructure in between.

To make matters more difficult, the features required for path MTU discovery are usually disabled by default in most infrastructures. Some, like AWS and GCP, allow you to enable ICMP traffic for cases like this but I don't believe the added complexity of dynamic MTU is worth the value. I believe the most reasonable thing to do is choose a default value that works with all systems and cloud providers and defer the cost and complexity of optimization to folks who have custom infrastructures that diverge from the standard.

@jmacd
Copy link
Contributor Author

jmacd commented Apr 14, 2020

(I had a default of 64kB because that used to work at my last employer.)

I take it 1432 is the value we want? Done.

@jmacd jmacd merged commit ea24ca8 into master Apr 14, 2020
@jmacd jmacd deleted the jmacd/restore_dogstatsd branch April 14, 2020 17:52
plantfansam referenced this pull request in plantfansam/opentelemetry-go-contrib Mar 18, 2022
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.

None yet

6 participants