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]

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

[eerhardt]

Suggested change

	exceptionTags.Add(new KeyValuePair<string, object?>(ExceptionStackTraceTag, exception.ToString()));
	// Note that "exception.stacktrace" is the full exception detail, not just the StackTrace property.
	// See https://opentelemetry.io/docs/specs/semconv/attributes-registry/exception/
	// and https://github.com/open-telemetry/opentelemetry-specification/pull/697#discussion_r453662519
	exceptionTags.Add(new KeyValuePair<string, object?>(ExceptionStackTraceTag, exception.ToString()));

[eerhardt]

Suggested change

	exceptionTags.Add(new KeyValuePair<string, object?>(ExceptionTypeTag, exception.GetType().ToString()));
	exceptionTags.Add(new KeyValuePair<string, object?>(ExceptionTypeTag, exception.GetType().FullName));

[eerhardt]

You should be able to just call .AddTag on the current Activity for each tag instead of adding a new event.

[aygalinc]

Opentelemetry spec states that exception should be recorded as event and not tag on the current activity

[aygalinc]

https://opentelemetry.io/docs/specs/otel/trace/exceptions/

[paulomorgado]

System.Diagnostics.DiagnosticSource 9.0.0 adds a AddException method to the Activity class.

You can see the implementation here.

Although it's safe to add a dependency to version 9.0.0 of this package, if you don't want to risk that, you can add it as an extension method to this project.

[aygalinc]

This recordException is basically a copy paste of the MCR method, I don't know if dependency bump is allowed

[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 ?