Skip to content

Commit

Permalink
[AspNetCore] [Http] remove Activity Status Description and update uni…
Browse files Browse the repository at this point in the history
…t tests (#5025)
  • Loading branch information
TimothyMothra authored Nov 9, 2023
1 parent 4a3c8d3 commit f25ff3a
Show file tree
Hide file tree
Showing 13 changed files with 34 additions and 83 deletions.
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Instrumentation.AspNetCore/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* Removed the Activity Status Description that was being set during
exceptions. Activity Status will continue to be reported as `Error`.
This is a **breaking change**. `EnrichWithException` can be leveraged
to restore this behavior.
([#5025](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5025))

* Updated `http.request.method` to match specification guidelines.
* For activity, if the method does not belong to one of the [known
values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,7 @@ public void OnException(Activity activity, object payload)
activity.RecordException(exc);
}

activity.SetStatus(ActivityStatusCode.Error, exc.Message);
activity.SetStatus(ActivityStatusCode.Error);

try
{
Expand Down
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Instrumentation.Http/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

## Unreleased

* Removed the Activity Status Description that was being set during
exceptions. Activity Status will continue to be reported as `Error`.
This is a **breaking change**. `EnrichWithException` can be leveraged
to restore this behavior.
([#5025](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5025))

* Updated `http.request.method` to match specification guidelines.
* For activity, if the method does not belong to one of the [known
values](https://github.com/open-telemetry/semantic-conventions/blob/v1.22.0/docs/http/http-spans.md#:~:text=http.request.method%20has%20the%20following%20list%20of%20well%2Dknown%20values)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ public void OnException(Activity activity, object payload)

if (exc is HttpRequestException)
{
activity.SetStatus(ActivityStatusCode.Error, exc.Message);
activity.SetStatus(ActivityStatusCode.Error);
}

try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -230,57 +230,30 @@ private static void AddExceptionTags(Exception exception, Activity activity, out
}

ActivityStatusCode status;
string exceptionMessage = null;

if (exception is WebException wexc)
if (exception is WebException wexc && wexc.Response is HttpWebResponse response)
{
if (wexc.Response is HttpWebResponse response)
{
statusCode = response.StatusCode;

if (tracingEmitOldAttributes)
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)statusCode);
}
statusCode = response.StatusCode;

if (tracingEmitNewAttributes)
{
activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode);
}

status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)statusCode);
if (tracingEmitOldAttributes)
{
activity.SetTag(SemanticConventions.AttributeHttpStatusCode, (int)statusCode);
}
else

if (tracingEmitNewAttributes)
{
switch (wexc.Status)
{
case WebExceptionStatus.Timeout:
case WebExceptionStatus.RequestCanceled:
status = ActivityStatusCode.Error;
break;
case WebExceptionStatus.SendFailure:
case WebExceptionStatus.ConnectFailure:
case WebExceptionStatus.SecureChannelFailure:
case WebExceptionStatus.TrustFailure:
case WebExceptionStatus.ServerProtocolViolation:
case WebExceptionStatus.MessageLengthLimitExceeded:
status = ActivityStatusCode.Error;
exceptionMessage = exception.Message;
break;
default:
status = ActivityStatusCode.Error;
exceptionMessage = exception.Message;
break;
}
activity.SetTag(SemanticConventions.AttributeHttpResponseStatusCode, (int)statusCode);
}

status = SpanHelper.ResolveSpanStatusForHttpStatusCode(activity.Kind, (int)statusCode);
}
else
{
status = ActivityStatusCode.Error;
exceptionMessage = exception.Message;
}

activity.SetStatus(status, exceptionMessage);
activity.SetStatus(status);

if (TracingOptions.RecordException)
{
activity.RecordException(exception);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_Old(

// Instrumentation is not expected to set status description
// as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode
if (!urlPath.EndsWith("exception"))
{
Assert.True(string.IsNullOrEmpty(activity.StatusDescription));
}
else
{
Assert.Equal("exception description", activity.StatusDescription);
}
Assert.Null(activity.StatusDescription);

if (recordException)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,14 +138,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_Dupe(

// Instrumentation is not expected to set status description
// as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode
if (!urlPath.EndsWith("exception"))
{
Assert.True(string.IsNullOrEmpty(activity.StatusDescription));
}
else
{
Assert.Equal("exception description", activity.StatusDescription);
}
Assert.Null(activity.StatusDescription);

if (recordException)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,7 @@ public async Task SuccessfulTemplateControllerCallGeneratesASpan_New(

// Instrumentation is not expected to set status description
// as the reason can be inferred from SemanticConventions.AttributeHttpStatusCode
if (!urlPath.EndsWith("exception"))
{
Assert.True(string.IsNullOrEmpty(activity.StatusDescription));
}
else
{
Assert.Equal("exception description", activity.StatusDescription);
}
Assert.Null(activity.StatusDescription);

if (recordException)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,7 @@ private static async Task HttpOutCallsAreCollectedSuccessfullyBodyAsync(

// Assert.Equal(tc.SpanStatus, d[span.Status.CanonicalCode]);
Assert.Equal(tc.SpanStatus, activity.Status.ToString());

if (tc.SpanStatusHasDescription.HasValue)
{
var desc = activity.StatusDescription;
Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(desc));
}
Assert.Null(activity.StatusDescription);

var normalizedAttributes = activity.TagObjects.Where(kv => !kv.Key.StartsWith("otel.")).ToDictionary(x => x.Key, x => x.Value.ToString());

Expand Down
2 changes: 0 additions & 2 deletions test/OpenTelemetry.Instrumentation.Http.Tests/HttpTestData.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,6 @@ public class HttpOutTestCase

public string SpanStatus { get; set; }

public bool? SpanStatusHasDescription { get; set; }

public Dictionary<string, string> SpanAttributes { get; set; }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -549,7 +549,7 @@ public async Task TestRequestWithException(string method)
Assert.Equal("Stop", exceptionEvent.Key);

Assert.True(activity.Status != ActivityStatusCode.Unset);
Assert.NotNull(activity.StatusDescription);
Assert.Null(activity.StatusDescription);
}

/// <summary>
Expand Down Expand Up @@ -627,7 +627,7 @@ public async Task TestSecureTransportFailureRequest(string method)
Assert.Equal("Stop", exceptionEvent.Key);

Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset);
Assert.NotNull(exceptionEvent.Value.StatusDescription);
Assert.Null(exceptionEvent.Value.StatusDescription);
}

/// <summary>
Expand Down Expand Up @@ -669,7 +669,7 @@ public async Task TestSecureTransportRetryFailureRequest(string method)
Assert.Equal("Stop", exceptionEvent.Key);

Assert.True(exceptionEvent.Value.Status != ActivityStatusCode.Unset);
Assert.NotNull(exceptionEvent.Value.StatusDescription);
Assert.Null(exceptionEvent.Value.StatusDescription);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,7 @@ public void HttpOutCallsAreCollectedSuccessfully(HttpTestData.HttpOutTestCase tc

if (tag.Key == SpanAttributeConstants.StatusDescriptionKey)
{
if (tc.SpanStatusHasDescription.HasValue)
{
Assert.Equal(tc.SpanStatusHasDescription.Value, !string.IsNullOrEmpty(tagValue));
}

Assert.Null(tagValue);
continue;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@
"url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/",
"spanName": "HTTP GET",
"spanStatus": "Error",
"spanStatusHasDescription": true,
"responseExpected": false,
"recordException": false,
"spanAttributes": {
Expand All @@ -111,7 +110,6 @@
"url": "http://sdlfaldfjalkdfjlkajdflkajlsdjf:{port}/",
"spanName": "HTTP GET",
"spanStatus": "Error",
"spanStatusHasDescription": true,
"responseExpected": false,
"recordException": true,
"spanAttributes": {
Expand Down

0 comments on commit f25ff3a

Please sign in to comment.