Add map of TCP failures to connections object #301

akarpz · 2024-04-22T14:00:13Z

What does this PR do?

Adds a new field to the Connections object in support of the TCP Failed Connections project.

Motivation

https://datadoghq.atlassian.net/browse/NPM-3297

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

Reviewers: please see the review guidelines.

leeavital · 2024-05-01T22:00:36Z

proto/process/connections.proto

@@ -184,6 +184,9 @@ message Connection {

  // serialized HTTP2Aggregations object summarizing all http2 transactions recorded for this connection, organized by request path
  bytes http2Aggregations = 50;
+
+  // serialized map of POSIX error codes (ex ETIMEOUT) to the number of times they have been encountered in the context of this connection


are these codes meant to be platform dependent, or is windows meant to approximate the POSIX error codes ?

Windows has said they can support the 3 initial codes we will support in linux. long term I don't think there needs to be platform parity, but I was also under the impression that POSIX codes were native to windows as well.

edit: I was wrong. I think we can ask windows to send the codes in POSIX format since they're just strings, or we can create our own format

POSIX APIs and errno's are available on Windows, but I think the question here is how the failed connections are implemented on Windows. I think we can standardize on POSIX error codes here.

How would you all feel about using a our own logical name for the error codes instead of potentially different Windows / Linux codes that mean the same thing? Instead of ETIMEOUT maybe represent it to the customer as DDTIMEOUT or something, just in case there is a difference between Windows / Linux. More of an idea than a suggestion - curious to hear thoughts.

If we come up with our own error code scheme, I'd recommend using an int enum instead of string as it will serialize more efficiently

Actually come to think of it I don't like the idea of the map key here being a string. I think this should be an int or an enum. enum seems more appealing to me, which means we should define our own codes. On linux they can map to POSIX error codes; Windows can do the same with whatever scheme they want to use to detect connection failures. This would also make it easier for the backend to interpret these errors.

works for me, I'll make the change

To clarify, @hmahmood / @akarpz - I might not be fully understanding. For the mapping, are we looking to have 1 enum that represents the logical behavior of the failed client side connection? As opposed to, a set of enums for Windows and for Linux?

The thing that is important to me here is that a failed TCP client connection should be displayed to the customer in the same way on Windows or on Linux. For example, if the connection is refused it should be CONNECTION_REFUSED and if the connection times out, it should be CONNECTION_TIMEDOUT, regardless of it came from Windows / Linux and regardless of how it was collected on Windows / Linux.

Probably will make sense when the example is up there.

This would be the reason to use our enums, which is what I was trying to say in my last comment.

The thing that is important to me here is that a failed TCP client connection should be displayed to the customer in the same way on Windows or on Linux. For example, if the connection is refused it should be CONNECTION_REFUSED and if the connection times out, it should be CONNECTION_TIMEDOUT, regardless of it came from Windows / Linux and regardless of how it was collected on Windows / Linux.

To address a few points:

I think the payload data should be platform agnostic. Windows and linux should set the same map keys and values, keeping an internal mapping if necessary.

In protobuf, enums can't be used as map keys. So we're left with using ints or strings.

I think it makes more sense to use ints. In linux, the errnos are ints (ie ETIMEOUT is 110). Storing them as strings in the payload means we have to map them from int -> string in the agent in userspace and store them that way on heap. Errnos are <256 I think, so we could even use int8 to store them in the agent and payload (which uses varint). At the scale of every connection, storing ints saves a decent amount of space vs strings.

If we use ints, windows would have to map whatever the internal representation of an error is from that to the corresponding errno int. I think this is cheap

On the other end, we could delay the conversion of int -> string (however we want to display it to the user), until writing to EVP or even the UI.

What does everyone think? cc: @leeavital @hmahmood @richl-dd

dustmop

LGTM for ASC

paulcacheux · 2024-05-09T08:08:42Z

cws/dumpsv1/activity_dump.pb.go

@@ -1,6 +1,6 @@
 // Code generated by protoc-gen-go. DO NOT EDIT.
 // versions:
-// 	protoc-gen-go v1.34.0


Is it really necessary to change this ?

the rakefile specifies latest, so every time you run rake codegen it may change these. I am in favor of hardcoding the version so version updates and functionality changes are not polluting each other's PRs

richl-dd · 2024-05-09T11:55:57Z

process/connections.pb.go

@@ -785,6 +785,8 @@ type Connection struct {
 	DataStreamsAggregations []byte `protobuf:"bytes,49,opt,name=dataStreamsAggregations,proto3" json:"dataStreamsAggregations,omitempty"`
 	// serialized HTTP2Aggregations object summarizing all http2 transactions recorded for this connection, organized by request path
 	Http2Aggregations []byte `protobuf:"bytes,50,opt,name=http2Aggregations,proto3" json:"http2Aggregations,omitempty"`
+	// serialized map of POSIX error codes to the number of times they have been encountered in the context of this connection (ex [110: 3] would mean that the connection has seen 3 ETIMEDOUT errors)


Is this PR POSIX specific or is this the general map of error codes across platforms?

I propose above that it should be POSIX, and windows maps from the internal windows representation of an error to the POSIX representation of it. This should be pretty easy as for now it's just 3 things. For example WINTIMEOUT would be 110.

akarpz · 2024-05-09T13:41:13Z

Rakefile

@@ -88,7 +88,7 @@ BASH
      mv v5/process/proto/process/*.go process

      # Install protoc-gen-go
-      GOPATH=#{protoc_gen_go_dir} go install google.golang.org/protobuf/cmd/protoc-gen-go@latest
+      GOPATH=#{protoc_gen_go_dir} go install google.golang.org/protobuf/cmd/protoc-gen-go@v1.34.0


@paulcacheux updated here

akarpz · 2024-05-17T14:36:42Z

/merge

dd-devflow · 2024-05-17T14:37:20Z

🚂 MergeQueue

Pull request added to the queue.

This build is going to start soon! (estimated merge in less than 0s)

Use /merge -c to cancel this operation!

dd-devflow · 2024-05-17T14:40:34Z

🚨 MergeQueue

Gitlab pipeline didn't start on its own and we were unable to create it... Please retry.

If you need support, contact us on Slack #devflow with those details!

akarpz added 3 commits April 22, 2024 09:59

add map of tcp failures to connections object

f191ace

codegen

0f3bfa1

Merge branch 'master' into akarpowich/tcp_failed_connections

4c99cb4

akarpz marked this pull request as ready for review May 1, 2024 18:19

akarpz requested review from a team as code owners May 1, 2024 18:19

akarpz changed the title ~~add map of tcp failures to connections object~~ Add map of TCP failures to connections object May 1, 2024

akarpz requested a review from a team May 1, 2024 21:57

leeavital reviewed May 1, 2024

View reviewed changes

dustmop approved these changes May 1, 2024

View reviewed changes

fisherevans approved these changes May 2, 2024

View reviewed changes

akarpz requested a review from a team May 2, 2024 15:57

change type to int

2880965

akarpz requested review from a team as code owners May 7, 2024 03:29

vboulineau approved these changes May 7, 2024

View reviewed changes

Merge branch 'master' into akarpowich/tcp_failed_connections

8d20df1

daniel-taf approved these changes May 8, 2024

View reviewed changes

akarpz requested a review from richl-dd May 8, 2024 21:10

paulcacheux reviewed May 9, 2024

View reviewed changes

richl-dd reviewed May 9, 2024

View reviewed changes

hardcode protoc-gen-go version

1edadef

akarpz commented May 9, 2024

View reviewed changes

akarpz requested review from paulcacheux and hmahmood May 10, 2024 18:23

hmahmood approved these changes May 10, 2024

View reviewed changes

Merge branch 'master' into akarpowich/tcp_failed_connections

c84785b

richl-dd approved these changes May 15, 2024

View reviewed changes

akarpz requested a review from Jakobovski May 15, 2024 14:46

akarpz removed the request for review from Jakobovski May 15, 2024 14:56

make map uint32

ac55806

akarpz mentioned this pull request May 15, 2024

Report TCP Failed Connections DataDog/datadog-agent#25517

Merged

dd-devflow bot added mergequeue-status: queued mergequeue-status: in_progress mergequeue-status: error and removed mergequeue-status: queued mergequeue-status: in_progress labels May 17, 2024

akarpz merged commit 2bd0df1 into master May 17, 2024
7 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add map of TCP failures to connections object #301

Add map of TCP failures to connections object #301

akarpz commented Apr 22, 2024 •

edited

Loading

leeavital May 1, 2024

akarpz May 1, 2024 •

edited

Loading

hmahmood May 2, 2024

richl-dd May 2, 2024

leeavital May 2, 2024

hmahmood May 2, 2024 •

edited

Loading

akarpz May 2, 2024

richl-dd May 3, 2024

hmahmood May 3, 2024

akarpz May 3, 2024 •

edited

Loading

dustmop left a comment

paulcacheux May 9, 2024

akarpz May 9, 2024

richl-dd May 9, 2024

akarpz May 10, 2024 •

edited

Loading

akarpz May 9, 2024

akarpz commented May 17, 2024

dd-devflow bot commented May 17, 2024

dd-devflow bot commented May 17, 2024

Add map of TCP failures to connections object #301

Add map of TCP failures to connections object #301

Conversation

akarpz commented Apr 22, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

Choose a reason for hiding this comment

akarpz May 1, 2024 • 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

hmahmood May 2, 2024 • 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

akarpz May 3, 2024 • edited Loading

Choose a reason for hiding this comment

dustmop 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

akarpz May 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akarpz commented May 17, 2024

dd-devflow bot commented May 17, 2024

dd-devflow bot commented May 17, 2024

akarpz commented Apr 22, 2024 •

edited

Loading

akarpz May 1, 2024 •

edited

Loading

hmahmood May 2, 2024 •

edited

Loading

akarpz May 3, 2024 •

edited

Loading

akarpz May 10, 2024 •

edited

Loading