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

Set Activity.Status when adding SetErrorStatusOnException #4336

Merged
merged 9 commits into from
Mar 31, 2023
Merged

Set Activity.Status when adding SetErrorStatusOnException #4336

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8d70a3d
Set Activity.Status and Mark SetStatus obsolete
vishweshbankwar Mar 27, 2023
0d2cd3c
Merge branch 'main' into vibankwa/fix-4333
vishweshbankwar Mar 27, 2023
a28e861
Set Activity.Status on SetErrorStatusOnException
vishweshbankwar Mar 27, 2023
084a83e
Merge branch 'vibankwa/fix-4333' of https://github.com/vishweshbankwa…
vishweshbankwar Mar 27, 2023
a858d9c
revert
vishweshbankwar Mar 27, 2023
5f41a23
Merge branch 'main' into vibankwa/fix-4333
vishweshbankwar Mar 28, 2023
5dff888
Add changelog
vishweshbankwar Mar 30, 2023
f11e508
Merge branch 'vibankwa/fix-4333' of https://github.com/vishweshbankwa…
vishweshbankwar Mar 30, 2023
3484019
Merge branch 'main' into vibankwa/fix-4333
vishweshbankwar Mar 30, 2023
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
4 changes: 4 additions & 0 deletions src/OpenTelemetry/Trace/ExceptionProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,11 @@ public override void OnEnd(Activity activity)

if (snapshot != pointers)
{
// TODO: Remove this when SetStatus is deprecated
utpilla marked this conversation as resolved.
Show resolved Hide resolved
activity.SetStatus(Status.Error);
Copy link
Member

@alanwest alanwest Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I assume there's never a time we'd call this version of SetStatus and not also want to call the "real" version, right?

Here's another spot

opentelemetry-dotnet/src/OpenTelemetry.Shims.OpenTracing/SpanBuilderShim.cs

Line 176 in a274f06

span.SetStatus(Status.Error);

We could kill two stones with one bird if we moved the call to activity.SetStatus(ActivityStatusCode) to our extension method. Though, from what I can tell I think that OpenTracing shim is the only other place... so maybe easier to just duplicate the call there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alanwest I was not aware of this one, will get it updated. what do you think about marking our extension method obsolete?

Copy link
Member

Choose a reason for hiding this comment

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

I would support marking it obsolete.

Copy link
Member

Choose a reason for hiding this comment

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

+1


// For processors/exporters checking `Status` property.
activity.SetStatus(ActivityStatusCode.Error);
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions test/OpenTelemetry.Tests/Trace/TracerProviderBuilderExtensionsTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public void SetErrorStatusOnExceptionEnabled()
}

Assert.Equal(StatusCode.Error, activity.GetStatus().StatusCode);
Assert.Equal(ActivityStatusCode.Error, activity.Status);
}

[Fact]
Expand Down Expand Up @@ -84,6 +85,7 @@ public void SetErrorStatusOnExceptionDisabled()
}

Assert.Equal(StatusCode.Unset, activity.GetStatus().StatusCode);
Assert.Equal(ActivityStatusCode.Unset, activity.Status);
}

[Fact]
Expand Down