Add Status field to Activity API

[cijothomas]

DiagnosticSource package was enhanced in 5.0 to achieve OpenTelemetry Tracing API scenarios. There were few areas which were required in OpenTelemetry API, but was not included in 5.0, due to timing issues/OpenTelemetry spec not being ready before .NET 5 cut off time.
This issue is about "Status" field in Activity. At the time of 5.0 , Status API in OpenTelemetry was not finalized. Now that the field and its intent is finalized in the OpenTelemetry spec, proposing to support this in Activity class.

Status spec in OpenTelemetry: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status

(Currently, OpenTelemetry works around by storing status inside special Activity.SetTag())

The proposal is added by @tarekgh

Proposal

    public enum ActivityStatusCode
    {
        Unset = 0, // This name picked up from the OTel specs.
        Ok = 1,
        Error = 2
    }

    public readonly struct ActivityStatus : IEquatable<ActivityStatus>
    {
        public ActivityStatus(ActivityStatusCode code, string? description = null);

        public static ActivityStatus Unset => new ActivityStatus(ActivityStatusCode.Unset);
        public static ActivityStatus Ok => new ActivityStatus(ActivityStatusCode.Ok);
        public static ActivityStatus Error => new ActivityStatus(ActivityStatusCode.Error);

        public ActivityStatusCode Code { get; }
        public string? Description { get; }

        public bool Equals(ActivityStatus value) => ...;
        public override bool Equals(object? obj) => ...;
        public static bool operator ==(ActivityStatus left, ActivityStatus right) => ...;
        public static bool operator !=(ActivityStatus left, ActivityStatus right) => ...;
        public override int GetHashCode() => ...;
    }

    public class Activity : IDisposable
    {
        ...
        public ActivityStatus Status { get; set; }
        ...
    }

Alternative Proposals

Alternative 1

This option is proposed to have consistency with the other patterns in the Activity class e.g. SetParentId, SetStartTime...etc.

    public class Activity : IDisposable
    {
        ...
        public ActivityStatus Status { get; } // getter only
        public Activity SetStatus(ActivityStatus status);
        ...
    }

Alternative 2

This option is proposed to have less writing when setting the status. like doing Activty.SetStatus(ActivityStatusCode.Ok, "Success") instead of Activity.Status = new ActivityStatus(ActivityStatusCode.Ok, "Success")

    public class Activity : IDisposable
    {
        ...
        public ActivityStatus Status { get; } // getter only
        public Activity SetStatus(ActivityStatusCode code, string? description);
        ...
    }

If we go with this option, we shouldn't provide the following static properties in the ActivityStatus.

        public static ActivityStatus Unset => new ActivityStatus(ActivityStatusCode.Unset);
        public static ActivityStatus Ok => new ActivityStatus(ActivityStatusCode.Ok);
        public static ActivityStatus Error => new ActivityStatus(ActivityStatusCode.Error);

Tagging subscribers to this area: @tarekgh, @tommcdon, @pjanotti
See info in area-owners.md if you want to be subscribed.

Issue Details

---

DiagnosticSource package was enhanced in 5.0 to achieve OpenTelemetry Tracing API scenarios. There were few areas which were required in OpenTelemetry API, but was not included in 5.0, due to timing issues/OpenTelemetry spec not being ready before .NET 5 cut off time.
This issue is about "Status" field in Activity. At the time of 5.0 , Status API in OpenTelemetry was not finalized. Now that the field and its intent is finalized in the OpenTelemetry spec, proposing to support this in Activity class.

Status spec in OpenTelemetry: https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status

(Currently, OpenTelemetry works around by storing status inside special Activity.SetTag())

Author:	cijothomas
Assignees:	-
Labels:	area-System.Diagnostics.Tracing, untriaged
Milestone:	-

[tarekgh]

@cijothomas @noahfalk I have added the proposed APIs in the issue description. Please have a look and send your feedback. Thanks!

[noahfalk]

ActivityStatusCode.Unset

I don't have any particular objection to the name, but I do think we should check with the API reviewers if a name such as 'None' would better follow .NET naming guidelines?

public ActivityStatus Status { get; set; }

Perhaps this should be?

public ActivityStatus Status { get; }
public void SetStatus(ActivityStatus status);

Two considerations lead me that direction:

1. Most of the other setters on Activity (though not all) follow this pattern
2. Many of the setters throw an exception if we are trying to set them at the wrong time and .NET guidelines say properties are not expected to throw.

Aside from that, LGTM!

[tarekgh]

@noahfalk

ActivityStatusCode.Unset

I don't have any particular objection to the name, but I do think we should check with the API reviewers if a name such as 'None' would better follow .NET naming guidelines?

I kept the same name as the specs defined it. But I am ok with whatever better name will be suggested by the design committee.

Most of the other setters on Activity (though not all) follow this pattern

Although this is true but we already introduced the property pattern like DisplayName. I am not really seeing a strong reason we need to stick to the pattern having getting property and setter method. It is really hard to follow and I never saw anyone recommending this pattern.

Many of the setters throw an exception if we are trying to set them at the wrong time and .NET guidelines say properties are not expected to throw.

This is not really accurate. The .NET design guidelines says the following:

✔️ DO preserve the previous value if a property setter throws an exception.

❌ AVOID throwing exceptions from property getters.

Property getters should be simple operations and should not have any preconditions. If a getter can throw an exception, it should probably be redesigned to be a method. Notice that this rule does not apply to indexers, where we do expect exceptions as a result of validating the arguments.

IMHO, I prefer the property than the setter method. Please let me know if you still think otherwise and I can add alternative design option to discuss with the design committee.

@cijothomas @reyang do you have any input on the proposal before we offer it to the design commitee?

[reyang]

I vote for using a method SetStatus and providing an optional description string:

Activity.SetStatus(StatusCode.Error, description: "connection reset");

Rather than these (since it is hard to use):

Activity.SetStatus(new ActivityStatus(code, description));

Activity.Status = new ActivityStatus(code, description);

https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status

Another thing we might consider - what's the behavior if the caller is using Activity.SetStatus(StatusCode.Unset) - should we allow it at all?

[cijothomas]

Activity.SetStatus(StatusCode.Error, description: "connection reset"); is my +1.