feat: add activity on connection

[aygalinc]

Proposed Changes

Add activity on connection creation.
In case of creation exception register an error event.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes issue #NNNN)
• [X ] New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Refer to #1731

[aygalinc]

@michaelklishin build seems broken due to some new CVE related to system text json ?

[lukebakken]

@eerhardt - do you have time to review this PR? Thanks!

[aygalinc]

@lukebakken when i implement the activity i create a new activity source but i m wondering if it s really usefull to have 3 ActivitySource and not use only one.

[eerhardt]

Is there any way to write some tests around this?

[eerhardt]

How/where does this activity get completed?

[aygalinc]

You are totally right. Missing some using keyword.
By reading the experimental activity on dotnet 9 http client I don't know if at some point we should link producer and consumer activity to the connection activity?

[eerhardt]

What do you mean by "should link producer and consumer activity"? My thinking was that creating the connection is the activity. Once the connection is created/established, the activity can be completed.

[paulomorgado]

Link how? Have the connection activity be the parent?

[aygalinc]

https://opentelemetry.io/docs/concepts/signals/traces/#span-links => here is what i mean by link

[eerhardt]

Suggested change

	if (!s_connectionSource.HasListeners())
	{
	return null;
	}

I don't think this is necessary. ActivitySource.CreateActivity will do this check and return null.

https://github.com/dotnet/runtime/blob/1622f514684d94a521bfb41c88a27079ad943ee7/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/ActivitySource.cs#L192-L200

[aygalinc]

I take the same logic as the rest of the implem but you are right when I read the Microsoft code this line is useless

[aygalinc]

I end up cleaning all has listener code

[lukebakken]

Thank you @eerhardt !!!

[lukebakken]

@aygalinc just FYI, I rebased this on main to pick up my test suite fixes. Hopefully GHA will be green going forward.

[aygalinc]

@eerhardt to the best of my knowledge Microsoft don't provide any testing support fort distributed tracing (contrary to metrics) . I do not find any test related to the other activity creation in rabbit. Maybe you have some resources that I miss to do this ?

[eerhardt]

@eerhardt to the best of my knowledge Microsoft don't provide any testing support fort distributed tracing (contrary to metrics) . I do not find any test related to the other activity creation in rabbit. Maybe you have some resources that I miss to do this ?

The main way I've seen it done is to hook listeners to the activity source, do the test, and then verify the activities were emitted as expected. Some examples:

1. dotnet/aspire: https://github.com/dotnet/aspire/blob/44948254b69ea9777f01081df0cb411e54797d33/tests/Aspire.Components.Common.Tests/ConformanceTests.cs#L429-L453
2. dotnet/aspnetcore: https://github.com/dotnet/aspnetcore/blob/67ae6c8b73e80fa5f499a069443c9844ca86c62a/src/SignalR/clients/csharp/Client/test/FunctionalTests/HubConnectionTests.Tracing.cs#L40-L56
3. dotnet/runtime: https://github.com/dotnet/runtime/blob/38366f093a26bc84c01704e107df467ef807666d/src/libraries/Common/tests/System/Net/ActivityRecorder.cs

[aygalinc]

@lukebakken do you now if it's possible to explicitly add System.Diagnostics.DiagnosticSource in version 9.0 in order to gain access to AddException method as suggested by @paulomorgado ?

[aygalinc]

@eerhardt I use the dotnet runtime approach in the test to capture Activity

[paulomorgado]

@lukebakken do you now if it's possible to explicitly add System.Diagnostics.DiagnosticSource in version 9.0 in order to gain access to AddException method as suggested by @paulomorgado ?

It's safe to add.

[aygalinc]

@eerhardt I check the code and I don't know what you envision on this use case : we can provide a list of endpoint the client will try to connect to one of them. Did we expect that each TCP connection will trigger a dedicated activity or we have a single activity with dedicated events that describe each TCP connection attempts?

[eerhardt]

I'm not sure. My naive thoughts are that the CreateConnectionAsync creates a single Activity, and each connection attempt inside of it would be a child Activity. But I haven't given it any major thought. https://github.com/open-telemetry/semantic-conventions/tree/main/docs/messaging doesn't seem to have conventions for connection Activities, so that isn't much help.

[aygalinc]

1 parent - 1 child activity per connection attempt : how an error is managed when the last attempt succeeded ? Individual Error should be attached to child activity but the activity who will be flag as error should be the parent one. In the actual design it is pretty complicated to have this behaviour from a bird eyes view. TCP connection establishment and open frame send are not so easy to correlate.

1 activity - 1 event per connection attempt: aggregated error is managed in the only activity. Activity does not contains so much useful information as they will be store in events (like ip, port etc) , can be a problem because some observability backend doesn't allowed to filter on event ( Tempo as example).