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

Add Datadog UDS support #123

Closed
wants to merge 5 commits into from
Closed

Conversation

sebcante
Copy link

@sebcante sebcante commented Sep 14, 2020

Goal

Adds Support for connecting to datadog via Unix Domain socket (UDS) instead of UDP

Why

UDS could be useful :

* Bypassing the networking stack brings a significant performance improvement for high traffic.
* While UDP has no error handling, UDS allows the Agent to detect dropped packets and connection errors, while still allowing a non-blocking use.
* DogStatsD can detect the container from which metrics originated and tag those metrics accordingly.

Taken from https://docs.datadoghq.com/developers/dogstatsd/unix_socket/?tab=host ^

Usage to connect to unixgram socket instead of udp

client := NewClient("unixgram:///dir/filename.sock") // unixgram

instead of

client := NewClient("localhost:8125") // udp

What

I hope this PR could be useful, completely open for feedbacks to align this PR to the conventions/style of this library. I gave it a first shot.
I inlined comments/questions

Credit

Datadog official client lib , i inspired a lot the code from it.

@sebcante sebcante changed the title Feature/uds support Datadog UDS support Sep 14, 2020
@sebcante sebcante changed the title Datadog UDS support Add Datadog UDS support Sep 14, 2020
return
}
defer f.Close()
func bufSizeFromFD(f *os.File, sizehint int) (bufsize int, err error) {
Copy link
Author

Choose a reason for hiding this comment

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

i would need an expert eye on this, i used the same existing logic against UDP for Unix socket too. I think that is right but i am not 100% sure. if not i can make this only for UDP?

if err != nil {
log.Printf("stats/datadog: %s", err)
log.Printf("stats/datadog: unable to create writer %s", err)
c.err = err
Copy link
Author

Choose a reason for hiding this comment

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

ideally i would return an error to the caller of NewClientWith if we can not create a writer, but decided to not change the API

newBufSize, err := w.CalcBufferSize(config.BufferSize)
if err != nil {
log.Printf("stats/datadog: unable to calc writer's buffer size. Defaulting to a buffer of size %d - %v\n", DefaultBufferSize, err)
newBufSize = DefaultBufferSize
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if we prefer seting newBufSize to 0 (existing behaviour)? i am open for suggestion

@sebcante sebcante marked this pull request as ready for review September 14, 2020 10:18
@tony-assembly
Copy link

is it possible to get someone to review this at all?

@sebcante
Copy link
Author

sebcante commented May 6, 2021

We have been using UDS support in production (this PR) for more than 6 months at mx51 it definitely helped us to better manage datadog agent maintenance and go apps reconnecting automatically to it. Something that UDP caused us trouble in the past.
@achille-roussel any thoughts if this contribution could be merged ?

kevinburkesegment added a commit that referenced this pull request Sep 9, 2024
I tried updating #123 with ~50
commits but the diff got too messy. Easier to just add the changes on
a new commit.
kevinburkesegment added a commit that referenced this pull request Oct 9, 2024
I tried updating #123 with ~50
commits but the diff got too messy. Easier to just add the changes on
a new commit.
@kevinburkesegment
Copy link
Contributor

Thank you for this! We just merged this upstream and it is part of the v5.1.0 release.

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