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

statsd: set client_transport properly #290

Merged
merged 6 commits into from
Dec 5, 2023

Conversation

iksaif
Copy link
Contributor

@iksaif iksaif commented Nov 27, 2023

This propagates the transport from the sender (and writer) to the telemetry client directly.

This introduces a new Transport interface with getTransportName() and teaches the telemetry client to read this and refresh its tags when necessary.

The result will be that:

  • 'unix://will useclient_transport:udsunless it connects to a UDS stream socket (and then switch toclient_transport:uds-stream`)
  • 'unixstream://will always directly useclient_transport:uds-stream`
  • No changes for the other writers

statsd/statsd.go Outdated Show resolved Hide resolved
statsd/statsd.go Show resolved Hide resolved
@vickenty
Copy link
Contributor

When unix:// is set, since we guess the name the type of the socket based on what is on the filesystem, we will use use client_transport:uds. Otherwise we will use client_trasnport:uds-stream or client_transport:uds-datagram

  1. Would it be possible to have the telemetry tag reflect actual socket type and not just configuration?
  2. Would it make sense to keep telemetry tags compatible with other clients and also backwards compatible? uds currently means "unix datagram sockets", can we keep it this way?

@iksaif
Copy link
Contributor Author

iksaif commented Nov 27, 2023

It's not really possible to do (1) because the tags are defined before the writer guesses the socket type.

The current implementation seems to be backward compatible with other clients and previous behavior in the sense that nothing will change for existing configurations (but more explicit configuration will allow us to have more explicit tagging).

In the long run I think it's a good incentive to have people move to unixstream:// or unixgram:// instead of just unix:// to make it clear what is the expected socket type

@iksaif
Copy link
Contributor Author

iksaif commented Dec 4, 2023

Ok I addressed the comment, and now unix datagram is reported as uds while steams are reported as uds-stream and this works even when the socket type is guessed

statsd/statsd.go Outdated Show resolved Hide resolved
statsd/uds.go Outdated Show resolved Hide resolved
statsd/telemetry.go Outdated Show resolved Hide resolved
statsd/uds.go Outdated Show resolved Hide resolved
@vickenty vickenty merged commit 7338de2 into DataDog:master Dec 5, 2023
23 checks passed
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.

2 participants