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);

[ghost]

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.

[tarekgh]

@reyang

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

Did you consider if someone has Status object and want to set on the Activity? that will be something

 Activity.SetStatus(status.Error, status.Description);

Which will look not that good compared to activity.Status = status. Also we are providing something like ActivityStatus.Ok, if you want to use it, it will look strange to write it as:

 Activity.SetStatus(ActivityStatus.Ok.Code, ActivityStatus.Ok.Description);

Why you think Activity.Status = new ActivityStatus(code, description); is not good compared to the one you proposed? Is it because using new?

If I go with your proposal, I would not provide at all.

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

I'll list all proposal as alternative options and we can discuss it in the design meeting.

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

I believe we should allow it for flexibility if there were some error and someone else wanted to ignore it. but I don't mind not allowing it if this can lead to wrong behavior.

[reyang]

Did you consider if someone has Status object and want to set on the Activity? that will be something

 Activity.SetStatus(status.Error, status.Description);

I guess nobody would do it? I don't have strong opinion here. Looks like Python is not taking this approach https://github.com/open-telemetry/opentelemetry-python/blob/6489bf50a576c9c772d2f7b78d677cf16b4526ac/opentelemetry-sdk/src/opentelemetry/sdk/trace/__init__.py#L670.

Why you think Activity.Status = new ActivityStatus(code, description); is not good compared to the one you proposed? Is it because using new?

I guess the SetStatus one requires less typing and looks tighter (no need to have two spaces around the =)?

If I go with your proposal, I would not provide at all.

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

There is a "cool way" which may take advantage of the predefined value https://github.com/open-telemetry/opentelemetry-dotnet/blob/8cda9ef394a1b075fd156d73dace48e48f5b3c9b/test/OpenTelemetry.Tests/Trace/ActivityExtensionsTest.cs#L54.
Not sure if it fits .NET framework style though.

[tarekgh]

There is a "cool way" which may take advantage of the predefined

Looks to me OTel is using SetStatus(Staus) and not the proposed SetStatus(code, description). no?

What I am trying to say is with the proposal SetStatus(code, description), the static properties wouldn't make sense. it would make sense with SetStatus(ActivityStatus) or Activity.Sattus = status.

I guess the SetStatus one requires less typing and looks tighter (no need to have two spaces around the =)?

I agree with that. I am just looking at the issue from the discoverability side. If someone look at Status property getter and notice no setter, it is difficult to guess there is a setter method.

I'll list all proposals so we ensure to discuss it in the review meeting. it is really good getting different opinions and compare between options.

[noahfalk]

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

Ah thanks for correcting my understanding. In that case I withdraw my objection, property setter is fine by me.

[terrajobst]

Video

• It seems problematic to have well-known fields/properties on ActivityStatus because it encourages code like activity.Status == ActivityStatus.Ok which would break if the description changes, which seems odd. At the same time, it would be odd for ActivityStatus's implementation of equality to ignore the description.
  • One option is to remove the well-known values from ActivityStatus
  • Another option is to remove ActivityStatus and expose code and description as separate properties on Activity. If we are concerned about extending the code, we can make ActivityStatusCode a struct with well known fields. The downside is that this allows for impossible states, such as an unset code but a non-null description.
• We concluded that two properties plus a method to throw on invalid combinations.

namespace System.Diagnostics
{
    public enum ActivityStatusCode
    {
        Unset = 0,
        Ok = 1,
        Error = 2
    }
    public partial class Activity
    {
        public ActivityStatus Status { get; }
        public string? StatusDescription { get; }
        public void SetStatus(ActivityStatusCode code, string? description = null);
    }
}

[tarekgh]

one small update to the approved API is having SetStatus returning activity object to be consistent with the rest of the Set methods inside the class.

public Activity SetStatus(ActivityStatusCode code, string? description = null);