Updating Status based on the new spec (#1313)

* Updating Status based on the new spec

commenting OtlpTrace.Status.Type check

fixing test

undoing change to statuscanonicalcode

fixing tests

reiley's comments

SetStatus now saves enum instead of string

fixing changes after conversion

fixing redis tests

from http spec, if 1xx,2xx,3xx => unset, otherwise error.
from rpc spec, if ok => unset, otherwise error.

fixing sql tests

fixing redis tests

* fixing sqlclient framework tests

* adding benchmark to status

* changing from hashset to simple if, readding statuscode for grpc

Author: eddynaka

examples/Console/TestRedis.cs

@@ -96,7 +96,7 @@ private static void DoWork(IDatabase db, ActivitySource activitySource)
                 catch (ArgumentOutOfRangeException e)
                 {
                     // Set status upon error
-                    activity.SetTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(Status.Internal.CanonicalCode));
+                    activity.SetTag(SpanAttributeConstants.StatusCodeKey, (int)Status.Error.StatusCode);
                     activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, e.ToString());
                 }
 

src/OpenTelemetry.Api/Trace/ActivityExtensions.cs

@@ -42,7 +42,7 @@ public static void SetStatus(this Activity activity, Status status)
         {
             Debug.Assert(activity != null, "Activity should not be null");
 
-            activity.SetTag(SpanAttributeConstants.StatusCodeKey, SpanHelper.GetCachedCanonicalCodeString(status.CanonicalCode));
+            activity.SetTag(SpanAttributeConstants.StatusCodeKey, (int)status.StatusCode);
             if (!string.IsNullOrEmpty(status.Description))
             {
                 activity.SetTag(SpanAttributeConstants.StatusDescriptionKey, status.Description);
@@ -66,9 +66,14 @@ public static Status GetStatus(this Activity activity)
 
             ActivityTagsEnumeratorFactory<ActivityStatusTagEnumerator>.Enumerate(activity, ref state);
 
-            var status = SpanHelper.ResolveCanonicalCodeToStatus(state.StatusCode);
+            if (!state.IsValid)
+            {
+                return default;
+            }
 
-            if (status.IsValid && !string.IsNullOrEmpty(state.StatusDescription))
+            var status = new Status(state.StatusCode);
+
+            if (!string.IsNullOrEmpty(state.StatusDescription))
             {
                 return status.WithDescription(state.StatusDescription);
             }
@@ -224,7 +229,9 @@ public bool ForEach(KeyValuePair<string, object> item)
 
         private struct ActivityStatusTagEnumerator : IActivityEnumerator<KeyValuePair<string, object>>
         {
-            public string StatusCode { get; private set; }
+            public bool IsValid { get; private set; }
+
+            public StatusCode StatusCode { get; private set; }
 
             public string StatusDescription { get; private set; }
 
@@ -233,14 +240,15 @@ public bool ForEach(KeyValuePair<string, object> item)
                 switch (item.Key)
                 {
                     case SpanAttributeConstants.StatusCodeKey:
-                        this.StatusCode = item.Value as string;
+                        this.StatusCode = (StatusCode)item.Value;
+                        this.IsValid = this.StatusCode == StatusCode.Error || this.StatusCode == StatusCode.Ok || this.StatusCode == StatusCode.Unset;
                         break;
                     case SpanAttributeConstants.StatusDescriptionKey:
                         this.StatusDescription = item.Value as string;
                         break;
                 }
 
-                return this.StatusCode == null || this.StatusDescription == null;
+                return this.IsValid || this.StatusDescription == null;
             }
         }
 

src/OpenTelemetry.Api/Trace/SpanHelper.cs

@@ -14,103 +14,29 @@
 // limitations under the License.
 // </copyright>
 
-using System;
-using System.Collections.Generic;
-
 namespace OpenTelemetry.Trace
 {
     /// <summary>
     /// A collection of helper methods to be used when building spans.
     /// </summary>
     public static class SpanHelper
     {
-#pragma warning disable CA1805 // Do not initialize unnecessarily
-        private static readonly Status DefaultStatus = default;
-#pragma warning restore CA1805 // Do not initialize unnecessarily
-        private static readonly Dictionary<StatusCanonicalCode, string> StatusCanonicalCodeToStringCache = new Dictionary<StatusCanonicalCode, string>()
-        {
-            [StatusCanonicalCode.Ok] = StatusCanonicalCode.Ok.ToString(),
-            [StatusCanonicalCode.Cancelled] = StatusCanonicalCode.Cancelled.ToString(),
-            [StatusCanonicalCode.Unknown] = StatusCanonicalCode.Unknown.ToString(),
-            [StatusCanonicalCode.InvalidArgument] = StatusCanonicalCode.InvalidArgument.ToString(),
-            [StatusCanonicalCode.DeadlineExceeded] = StatusCanonicalCode.DeadlineExceeded.ToString(),
-            [StatusCanonicalCode.NotFound] = StatusCanonicalCode.NotFound.ToString(),
-            [StatusCanonicalCode.AlreadyExists] = StatusCanonicalCode.AlreadyExists.ToString(),
-            [StatusCanonicalCode.PermissionDenied] = StatusCanonicalCode.PermissionDenied.ToString(),
-            [StatusCanonicalCode.ResourceExhausted] = StatusCanonicalCode.ResourceExhausted.ToString(),
-            [StatusCanonicalCode.FailedPrecondition] = StatusCanonicalCode.FailedPrecondition.ToString(),
-            [StatusCanonicalCode.Aborted] = StatusCanonicalCode.Aborted.ToString(),
-            [StatusCanonicalCode.OutOfRange] = StatusCanonicalCode.OutOfRange.ToString(),
-            [StatusCanonicalCode.Unimplemented] = StatusCanonicalCode.Unimplemented.ToString(),
-            [StatusCanonicalCode.Internal] = StatusCanonicalCode.Internal.ToString(),
-            [StatusCanonicalCode.Unavailable] = StatusCanonicalCode.Unavailable.ToString(),
-            [StatusCanonicalCode.DataLoss] = StatusCanonicalCode.DataLoss.ToString(),
-            [StatusCanonicalCode.Unauthenticated] = StatusCanonicalCode.Unauthenticated.ToString(),
-        };
-
-        /// <summary>
-        /// Helper method that returns the string version of a <see cref="StatusCanonicalCode"/> using a cache to save on allocations.
-        /// </summary>
-        /// <param name="statusCanonicalCode"><see cref="StatusCanonicalCode"/>.</param>
-        /// <returns>String version of the supplied <see cref="StatusCanonicalCode"/>.</returns>
-        public static string GetCachedCanonicalCodeString(StatusCanonicalCode statusCanonicalCode)
-        {
-            if (!StatusCanonicalCodeToStringCache.TryGetValue(statusCanonicalCode, out string canonicalCode))
-            {
-                return statusCanonicalCode.ToString();
-            }
-
-            return canonicalCode;
-        }
-
         /// <summary>
         /// Helper method that populates span properties from http status code according
-        /// to https://github.com/open-telemetry/opentelemetry-specification/blob/2316771e7e0ca3bfe9b2286d13e3a41ded6b8858/specification/data-http.md.
+        /// to https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/semantic_conventions/http.md#status.
         /// </summary>
         /// <param name="httpStatusCode">Http status code.</param>
         /// <returns>Resolved span <see cref="Status"/> for the Http status code.</returns>
         public static Status ResolveSpanStatusForHttpStatusCode(int httpStatusCode)
         {
-            var newStatus = Status.Unknown;
+            var status = Status.Error;
 
             if (httpStatusCode >= 100 && httpStatusCode <= 399)
             {
-                newStatus = Status.Ok;
-            }
-            else if (httpStatusCode == 400)
-            {
-                newStatus = Status.InvalidArgument;
-            }
-            else if (httpStatusCode == 401)
-            {
-                newStatus = Status.Unauthenticated;
-            }
-            else if (httpStatusCode == 403)
-            {
-                newStatus = Status.PermissionDenied;
-            }
-            else if (httpStatusCode == 404)
-            {
-                newStatus = Status.NotFound;
-            }
-            else if (httpStatusCode == 429)
-            {
-                newStatus = Status.ResourceExhausted;
-            }
-            else if (httpStatusCode == 501)
-            {
-                newStatus = Status.Unimplemented;
-            }
-            else if (httpStatusCode == 503)
-            {
-                newStatus = Status.Unavailable;
-            }
-            else if (httpStatusCode == 504)
-            {
-                newStatus = Status.DeadlineExceeded;
+                status = Status.Unset;
             }
 
-            return newStatus;
+            return status;
         }
 
         /// <summary>
@@ -121,51 +47,17 @@ public static Status ResolveSpanStatusForHttpStatusCode(int httpStatusCode)
         /// <returns>Resolved span <see cref="Status"/> for the Grpc status code.</returns>
         public static Status ResolveSpanStatusForGrpcStatusCode(int statusCode)
         {
-            var newStatus = Status.Unknown;
+            var status = Status.Error;
 
             if (typeof(StatusCanonicalCode).IsEnumDefined(statusCode))
             {
-                newStatus = new Status((StatusCanonicalCode)statusCode);
+                status = ((StatusCanonicalCode)statusCode) switch
+                {
+                    StatusCanonicalCode.Ok => Status.Unset,
+                    _ => Status.Error,
+                };
             }
 
-            return newStatus;
-        }
-
-        /// <summary>
-        /// Helper method that returns Status from <see cref="StatusCanonicalCode"/> to save on allocations.
-        /// </summary>
-        /// <param name="statusCanonicalCode"><see cref="StatusCanonicalCode"/>.</param>
-        /// <returns>Resolved span <see cref="Status"/> for the Canonical status code.</returns>
-        public static Status ResolveCanonicalCodeToStatus(string statusCanonicalCode)
-        {
-            bool success = Enum.TryParse(statusCanonicalCode, out StatusCanonicalCode canonicalCode);
-
-            if (!success)
-            {
-                return DefaultStatus;
-            }
-
-            var status = canonicalCode switch
-            {
-                StatusCanonicalCode.Cancelled => Status.Cancelled,
-                StatusCanonicalCode.Unknown => Status.Unknown,
-                StatusCanonicalCode.InvalidArgument => Status.InvalidArgument,
-                StatusCanonicalCode.DeadlineExceeded => Status.DeadlineExceeded,
-                StatusCanonicalCode.NotFound => Status.NotFound,
-                StatusCanonicalCode.AlreadyExists => Status.AlreadyExists,
-                StatusCanonicalCode.PermissionDenied => Status.PermissionDenied,
-                StatusCanonicalCode.ResourceExhausted => Status.ResourceExhausted,
-                StatusCanonicalCode.FailedPrecondition => Status.FailedPrecondition,
-                StatusCanonicalCode.Aborted => Status.Aborted,
-                StatusCanonicalCode.OutOfRange => Status.OutOfRange,
-                StatusCanonicalCode.Unimplemented => Status.Unimplemented,
-                StatusCanonicalCode.Internal => Status.Internal,
-                StatusCanonicalCode.Unavailable => Status.Unavailable,
-                StatusCanonicalCode.DataLoss => Status.DataLoss,
-                StatusCanonicalCode.Unauthenticated => Status.Unauthenticated,
-                _ => Status.Ok,
-            };
-
             return status;
         }
     }