Add UDS support #2

kang-makes · 2023-08-01T14:27:40Z

Description:

Link to tracking Issue:

Testing:

Documentation:

sigilioso

Looks good!

receiver/statsdreceiver/receiver_test.go

receiver/statsdreceiver/transport/server_test.go

sigilioso · 2023-08-02T06:56:36Z

receiver/statsdreceiver/transport/packet_server.go

+			addr = &net.UnixAddr{
+				Net:  "unixgram",
+				Name: "UDS",
+			}


Would it make sense to include a comment explaining the hardcoded address value for unixgram address?

At the beginning I agreed with you, but after checking the code, there are almost no if-else due to that difference

I saw no way to check for the transport of PacketConn to not hardcode so I did it. I would like to know what upstream thinks about this.

I left my comment in the wrong conversation 🤦 it was meant to go below "Did you consider having two Server types udpServer and unixGramServer and creating one or the other depending on transport?"

You have the answer to that here: #2 (comment)

sigilioso · 2023-08-02T07:02:45Z

receiver/statsdreceiver/transport/packet_server.go

+	packetConn net.PacketConn
+	transport  Transport
+	address    string
+}


Did you consider having two Server types udpServer and unixGramServer and creating one or the other depending on transport?

Because of the issue od addr being nil only when using a Unix socket I saw more difficult to have that part separated than having everything on the same file.

paologallinaharbur · 2023-08-03T08:44:04Z

receiver/statsdreceiver/testutil/temporary_socket.go

+	"testing"
+)
+
+func CreateTemporarySocket(t testing.TB, _ string) string {


why do we have the second argument if we ignore it?

isn't easier to have a fixed name if tempDir() is always changing?

or you can leverage createTemp https://pkg.go.dev/os#CreateTemp

Because the signature has to be compatible with https://github.com/kang-makes/opentelemetry-collector-contrib/blob/receiver/statsdreceiver/add-uds-support-part-2/internal/common/testutil/testutil.go#L35

I also used t.TempDir() because the testing suite cleans all files after finishing tests so we can keep the house clean :)

paologallinaharbur · 2023-08-03T08:50:30Z

receiver/statsdreceiver/transport/client/client.go

+		unixAddr, err := net.ResolveUnixAddr(s.transport, s.address)
+		if err != nil {
+			return err
+		}
+		s.Conn, err = net.DialUnix(s.transport, nil, unixAddr)
+		if err != nil {
+			return err
+		}


I would have two functions:

connectUDP (including two methods above)

connectUnixgram(including two methods below)

so that this one becomes more readable. Moreover I would wrap errors

paologallinaharbur · 2023-08-03T08:53:24Z

receiver/statsdreceiver/transport/transport.go

+
+// Returns true if the transport is unix socket based.
+func (trans Transport) IsUnixTransport() bool {
+	switch trans {


NIT isn't the switch an overkill?

return trans == UnixGram maybe?

You are missing protocols that can appear like unix, unixgram... I kept the same structure for all functions.

paologallinaharbur · 2023-08-03T09:05:25Z

receiver/statsdreceiver/transport/packet_server.go

+var _ (Server) = (*packetServer)(nil)
+
+// NewPacketServer creates a transport.Server using transports based on packets.
+func NewPacketServer(transport Transport, address string) (Server, error) {


is address still the right term?

Good question. I had issues finding new wording that covers an IP address and a socket path... socket address sounds good to me but I am open to suggestions.

receiver/statsdreceiver/transport/packet_server.go

paologallinaharbur · 2023-08-03T09:11:39Z

receiver/statsdreceiver/transport/packet_server.go

+			var netErr net.Error
+			reporter.OnDebugf("%s Transport (%s) - ReadFrom error: %v", psrv.transport, psrv.packetConn.LocalAddr(), err)
+			if errors.As(err, &netErr) {
+				if netErr.Timeout() {
+					continue
+				}
+			}
+			return err


would make senso to have handleError as we have handlePacket?

This has exactly the same structure as the receiver had before my refactor. If you do not go commit by commit you'll see a whole new file but if you do you see that I did not make any change on that part.

receiver/statsdreceiver/transport/packet_server.go

paologallinaharbur

Just nits, ignore what you do not agree with! 😄

**Description:** This is the first step to adding UDS support to the StatsD receiver. As I started to develop it, I saw that all tests were hardcoded to UDP and using networking and there was no possibility to add a socket communication. PR started to get huge so I split it into two, one to refactor tests and another one that adds UDS support: * Removed all unused references. * Made a `Transport` helper that allows centralizing all supported protocols and constants in its package. * Removed all hardcoded UDP protocols and generalized testing so new protocols are easy to add. If you need a rationale about why the changes are like this, this is the next PR I am going to submit after this one is merged: kang-makes#2 That is the PR that is going to add UDS support properly. **Link to tracking Issue:** - #21385 **Previous closed PR:** - #24832

…28896) **Description:** This is the first step to adding UDS support to the StatsD receiver. As I started to develop it, I saw that all tests were hardcoded to UDP and using networking and there was no possibility to add a socket communication. PR started to get huge so I split it into two, one to refactor tests and another one that adds UDS support: * Removed all unused references. * Made a `Transport` helper that allows centralizing all supported protocols and constants in its package. * Removed all hardcoded UDP protocols and generalized testing so new protocols are easy to add. If you need a rationale about why the changes are like this, this is the next PR I am going to submit after this one is merged: kang-makes#2 That is the PR that is going to add UDS support properly. **Link to tracking Issue:** - open-telemetry#21385 **Previous closed PR:** - open-telemetry#24832

jcogilvie · 2024-11-25T19:23:43Z

Any chance this can get PR'd upstream?

kang-makes · 2024-12-01T20:17:51Z

Hi @jcogilvie,

The PR is stale. At the time I wrote this, tests were coupled and had hardcoded responeses that needed a refactor before this one. Hence #1

The refactor for tests was asked upstream (open-telemetry#24832), it was rebased 3 times and completely ignored even after going to the collector meetings begging for this to be merged.

The collector moved in this year and the PR need a full refactor from scratch (again). I am done with this and I will not work on this anymore.

Closing.

sigilioso mentioned this pull request Aug 2, 2023

Refactor tests to remove all UDP that is hardcoded #1

Closed

sigilioso reviewed Aug 2, 2023

View reviewed changes

paologallinaharbur reviewed Aug 3, 2023

View reviewed changes

receiver/statsdreceiver/transport/packet_server.go Outdated Show resolved Hide resolved

paologallinaharbur reviewed Aug 3, 2023

View reviewed changes

receiver/statsdreceiver/transport/packet_server.go Outdated Show resolved Hide resolved

paologallinaharbur reviewed Aug 3, 2023

View reviewed changes

receiver/statsdreceiver/transport/packet_server.go Outdated Show resolved Hide resolved

paologallinaharbur approved these changes Aug 3, 2023

View reviewed changes

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support-part-2 branch 3 times, most recently from 03a724e to 107114b Compare August 3, 2023 13:34

kang-makes mentioned this pull request Aug 3, 2023

[receiver/statsd] Refactor of hardcoded tests open-telemetry/opentelemetry-collector-contrib#24832

Closed

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from 59faed9 to 7094a68 Compare August 21, 2023 10:52

kang-makes added 10 commits August 22, 2023 11:40

refactor of the tests so they are not hardcoded

3389085

address christian comments

448cdb0

address marc comments

42833bb

address paolo comments

d13cb5f

add changelog

3af5309

remove leftovers

1bf88d0

return err only if it is an error

f1848f7

fix linter license

33b1172

remove more leftovers

013f145

remove changelog

e4f8238

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from 7094a68 to e4f8238 Compare August 22, 2023 09:40

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support-part-2 branch from 107114b to 4ae630e Compare August 22, 2023 12:54

kang-makes added 2 commits August 22, 2023 14:58

change UDP server name to a generic one

01c8ed2

Add UDS support to the receiver

ed30a21

kang-makes added 5 commits August 22, 2023 14:58

fix rebase shenanigans

c7a82c1

add changelog

bfa9443

refactor client to test util package

ed1819b

address internal comments

beb01bf

fix goporto imports

116da7b

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support-part-2 branch from 4ae630e to 116da7b Compare August 22, 2023 12:59

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from e4f8238 to 4fa23ed Compare September 7, 2023 13:46

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch 4 times, most recently from c6f0e45 to 7b39dc2 Compare November 6, 2023 08:30

kang-makes mentioned this pull request Nov 6, 2023

[chore][receiver/statsd] Refactor of hardcoded tests open-telemetry/opentelemetry-collector-contrib#28896

Merged

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch from 7b39dc2 to 892ed4f Compare November 6, 2023 09:27

kang-makes force-pushed the receiver/statsdreceiver/add-uds-support branch 2 times, most recently from 7965808 to 51be5d3 Compare November 21, 2023 10:43

michaelli321 mentioned this pull request Dec 1, 2024

Add Unix Domain Socket Listener to statsdreceiver open-telemetry/opentelemetry-collector-contrib#21385

Open

kang-makes closed this Dec 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UDS support #2

Add UDS support #2

kang-makes commented Aug 1, 2023

sigilioso left a comment

sigilioso Aug 2, 2023

paologallinaharbur Aug 3, 2023

kang-makes Aug 3, 2023

paologallinaharbur Aug 3, 2023 •

edited

Loading

kang-makes Aug 3, 2023

sigilioso Aug 2, 2023

kang-makes Aug 3, 2023

paologallinaharbur Aug 3, 2023

paologallinaharbur Aug 3, 2023

paologallinaharbur Aug 3, 2023

kang-makes Aug 3, 2023

paologallinaharbur Aug 3, 2023 •

edited

Loading

paologallinaharbur Aug 3, 2023

sigilioso Aug 3, 2023

kang-makes Aug 3, 2023 •

edited

Loading

paologallinaharbur Aug 3, 2023

kang-makes Aug 3, 2023

paologallinaharbur Aug 3, 2023

kang-makes Aug 3, 2023

paologallinaharbur left a comment

jcogilvie commented Nov 25, 2024

kang-makes commented Dec 1, 2024

Add UDS support #2

Add UDS support #2

Conversation

kang-makes commented Aug 1, 2023

sigilioso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paologallinaharbur Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paologallinaharbur Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kang-makes Aug 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paologallinaharbur left a comment

Choose a reason for hiding this comment

jcogilvie commented Nov 25, 2024

kang-makes commented Dec 1, 2024

paologallinaharbur Aug 3, 2023 •

edited

Loading

paologallinaharbur Aug 3, 2023 •

edited

Loading

kang-makes Aug 3, 2023 •

edited

Loading