Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Updating Status based on the new spec #1313

Merged
merged 6 commits into from
Oct 13, 2020
Merged

Updating Status based on the new spec #1313

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
009d63e
Updating Status based on the new spec
eddynaka Sep 29, 2020
74d8646
fixing sqlclient framework tests
eddynaka Oct 12, 2020
454fddc
Merge branch 'master' into feature/breaking-status
eddynaka Oct 13, 2020
bbb76e2
adding benchmark to status
eddynaka Oct 13, 2020
ce48282
Merge branch 'feature/breaking-status' of https://github.com/eddynaka…
eddynaka Oct 13, 2020
7a01570
changing from hashset to simple if, readding statuscode for grpc
eddynaka Oct 13, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/Console/TestRedis.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

Expand Down
20 changes: 14 additions & 6 deletions src/OpenTelemetry.Api/Trace/ActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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; }

Expand All @@ -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 = Enum.IsDefined(typeof(StatusCode), item.Value);
eddynaka marked this conversation as resolved.
Show resolved Hide resolved
break;
case SpanAttributeConstants.StatusDescriptionKey:
this.StatusDescription = item.Value as string;
break;
}

return this.StatusCode == null || this.StatusDescription == null;
return this.IsValid || this.StatusDescription == null;
}
}

Expand Down
128 changes: 10 additions & 118 deletions src/OpenTelemetry.Api/Trace/SpanHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in what gRPC is doing, but this seems off to me. Just took a quick look at the code it seems to set a status code in grpc.status_code, then it sets status from that, and then it removes grpc.status_code. This new version, I feel like we are going to lose some fidelity that was there previously. Maybe we should set status as it is done here, but leave grpc.status_code set to the raw "canonicalcode" value? Maybe we should also move StatusCanonicalCode into gRPC library since it is no longer in line with the spec.

/cc @alanwest

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from this part, I'm still waiting a formal change in the spec to see.

I think I didn't change the part where it gets the value from the activity, so it might remain in the same way (the canonicalcode one). And, for the status itself, yeah, that will have only the 3 possibilities from spec.

Still waiting to confirm its behavior...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess most likely the gRPC code will be stored in a similar fashion as http.status_code since it has more semantic than the Unset/Ok/Error value.

I would suggest that we merge this PR as-is since the API part looks good, and we can catch the 0.7.0 release train.
The gRPC semantic convention is a lower priority comparing to the API since it is not a blocking issue, and it can be part of the 0.8.0 release or later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, GRPC already fills the status in the tag as grpc.status_code. So, it won't be a problem at all 👍

Yes, I think we can proceed and merge. Right now, the only part that is not "done" is the rpc part in the spec, since it will change because it was not updated in the first place.

Removing the label and adding to be merged asap

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc.status_code is added by the library and the removal of the attribute in favor of the spec-approved attribute is intentional. See #1021.

Are we adding back grpc.status_code until the spec change gets sorted out?

I see a suggestion for something like rpc.grpc.status_code here open-telemetry/opentelemetry-specification#1044 (comment).

I agree with @CodeBlanch that it will likely make sense to bring the StatusCanonicalCode into the gRPC instrumentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @alanwest , it would be strange to remove one tag and add another tag which contains the same value...i would just enable it and try to set that from spec side. for example, so we would need to just enable it in our side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just updated the issue to maintain grpc.status_code. With that, we will just have to remove the SetTag => null for that key.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just enabled and i will create a new issue so we can follow the changes in the spec


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;
}
}
Expand Down
Loading