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 .NET network + HTTP connection spans #1192

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Jun 27, 2024

This PR introduces additional network-level spans to-be emitted by .NET 9 🤞

Related to #454, #1226

In particular:

  1. DNS, TLS, and socket connect spans that track individual stages of connection establishment
  2. wrapper HTTP connection setup span for above spans
  3. wait-for-connection span that signals when a connection is not immediately available for the request
    • link that connects wait-for-connection to the connection setup
    • is expected to be off by default
    • TODO: document that it should be opt-in.

On .NET every ActivitySource (aka tracer) is opt-in, so users have full control over what they are enabling.

The volume of new telemetry is expected to be low (with caveats):

image

Overall, the feature is intended to help debug issues like:

  • misconfigured connection pool
  • DNS/TLS
  • abandoned response streams (or slow response stream reading) that result in connection staying busy
  • ...

docs/dotnet/dotnet-connection-traces.md Outdated Show resolved Hide resolved
Comment on lines 274 to 322
Span name SHOULD be `DNS {dns.question.name}`.
Span kind SHOULD be `CLIENT`

Corresponding `Activity.OperationName` is `System.Net.NameResolution.DnsLookup`, `ActivitySource` name - `System.Net.NameResolution`.

Choose a reason for hiding this comment

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

Should a future custom DNS resolver, emit the same span? Should it come from the same activity source?

Copy link

@antonfirsov antonfirsov Jul 5, 2024

Choose a reason for hiding this comment

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

@lmolkova actually, this is even more tricky. The current System.Net.Dns telemetry (including the dns.lookup.duration metric) covers protocol-independent lookup methods (eg. reverse lookup).

It would make sense to include those in our spans as well. We have to figure out our naming strategy. We can keep everything under the dns* terminology but document that it may cover more, or find an alternative term, like resolver.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the description, could you please check if it's addressed now?

(also would appreciate span names suggestions) for forward/reverse lookup.

docs/dotnet/dotnet-connection-traces.md Outdated Show resolved Hide resolved
@lmolkova lmolkova force-pushed the dotnet-http-connection-spans branch from 7d7c232 to 9a4ba14 Compare July 3, 2024 00:59
@lmolkova lmolkova force-pushed the dotnet-http-connection-spans branch from 9a4ba14 to eb80b71 Compare July 6, 2024 03:51
@lmolkova lmolkova force-pushed the dotnet-http-connection-spans branch from f6f4a99 to 0e56f65 Compare July 8, 2024 19:49
@lmolkova lmolkova changed the title Add .NET HTTP connection spans Add .NET network + HTTP connection spans Jul 8, 2024
@lmolkova lmolkova marked this pull request as ready for review July 8, 2024 23:16
@lmolkova lmolkova requested review from a team July 8, 2024 23:16
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
| [`error.type`](/docs/attributes-registry/error.md) | string | Socket error code. [1] | `connection_refused`; `address_not_available` | `Conditionally Required` if and only if an error has occurred. | ![Stable](https://img.shields.io/badge/-stable-lightgreen) |
| [`network.peer.address`](/docs/attributes-registry/network.md) | string | Peer address of the network connection - IP address or Unix domain socket name. | `10.1.2.80`; `/tmp/my.sock` | `Recommended` | ![Stable](https://img.shields.io/badge/-stable-lightgreen) |
| [`network.peer.port`](/docs/attributes-registry/network.md) | int | Peer port number of the network connection. | `65123` | `Recommended` | ![Stable](https://img.shields.io/badge/-stable-lightgreen) |
| [`network.type`](/docs/attributes-registry/network.md) | string | [OSI network layer](https://osi-model.com/network-layer/) or non-OSI equivalent. [2] | `ipv4`; `ipv6` | `Recommended` | ![Stable](https://img.shields.io/badge/-stable-lightgreen) |

Choose a reason for hiding this comment

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

Similarly, needs a comment that this is IP-only.

It's unclear what should we report for UDS from the network.* attribute list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should report network.transport for unix domain sockets. Could we return it (report it if it's not TCP)?

Choose a reason for hiding this comment

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

Replied in dotnet/runtime#103922 (comment) so networking folks can see the discussion.

docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Outdated Show resolved Hide resolved
@lmolkova lmolkova force-pushed the dotnet-http-connection-spans branch from ae799fd to 46d772f Compare July 11, 2024 06:41
docs/dotnet/dotnet-network-traces.md Show resolved Hide resolved
docs/dotnet/dotnet-network-traces.md Show resolved Hide resolved
Span name SHOULD be `TLS client handshake {server.address}` when authenticating on the client side and `TLS server handshake` when authenticating the server.
Span kind SHOULD be `INTERNAL` in both cases.

When *TLS* span is reported for client-side authentication along with *HTTP connection setup* and *Socket connect* span, the *TLS* span becomes a child of *HTTP connection setup*.

Choose a reason for hiding this comment

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

If I'm not mistaken, isn't the TLS handshake part of the socket creation process? If that's the case, should it be the child of that span if it exists?

Choose a reason for hiding this comment

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

No, System.Net.Sockets.Socket is a thin wrapper over OS sockets, which do not do encryption.

Choose a reason for hiding this comment

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

But you can't write to a secure socket until the TLS handshake is done, right? I suppose if you're modelling the socket span as being representative of the creation of the socket but before protocol configuration is done, thus allowing it to be used to transmit data, this makes sense.

I suppose my world view is influenced by how OkHttp's lifecycle events model this, bundling the configuration with the creation given a secure socket isn't truly ready to go until the TLS handshake is done.

If this makes more sense for the .NET, fair enough 😅

Copy link

@antonfirsov antonfirsov Jul 12, 2024

Choose a reason for hiding this comment

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

bundling the configuration with the creation given a secure socket isn't truly ready to go until the TLS handshake is done.

We model the creation period with the new spans, or in our terminology, "connection setup".
There is no such thing as "secure socket" in lower level layers. The counterpart that best matches the "creation of a secure socket" is our "HTTP connection_setup" span:

image

Note: TLS client has been renamed to TLS client handshake since I took that screenshot.

@codefromthecrypt
Copy link

@trustin @swankjesse @minwoox I know this is a tall order, but I'll ask anyway since years ago you all had helped with this and led to zipkin annotations or otherwise.

Do you have some cycles to review otel semantics (aka data recording conventions like tag names which are called attributes in otelngish)? I know you have all very deep work in interception and lifecycle of HTTP and there are some subtleties here which are more than meets the eye 🤖

Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 11, 2024
@lmolkova lmolkova removed the Stale label Aug 11, 2024
CodeBlanch pushed a commit to CodeBlanch/semantic-conventions that referenced this pull request Aug 16, 2024
* Remove absolute links where possible.

* Work around semantic conventions.

* Docfx.

* Fix YAML.

* More docfx.
CodeBlanch pushed a commit to CodeBlanch/semantic-conventions that referenced this pull request Aug 16, 2024
* Remove absolute links where possible.

* Work around semantic conventions.

* Docfx.

* Fix YAML.

* More docfx.
Copy link

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Aug 27, 2024
Copy link

github-actions bot commented Sep 3, 2024

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Sep 3, 2024
@lmolkova lmolkova reopened this Sep 3, 2024
@lmolkova
Copy link
Contributor Author

lmolkova commented Sep 3, 2024

@antonfirsov could you please review and approve if it aligns with the implementation? Thanks!

@antonfirsov antonfirsov self-assigned this Sep 3, 2024
@github-actions github-actions bot removed the Stale label Sep 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants