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

[api] Mark SetStatus & GetStatus ActivityExtensions obsolete #5781

Merged
merged 21 commits into from
Aug 21, 2024
Merged

[api] Mark SetStatus & GetStatus ActivityExtensions obsolete #5781

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
0193320
Mark ActivityExtensions.SetStatus obsolete
ysolomchenko Aug 9, 2024
3dde1fc
use ActivityStatusCode
ysolomchenko Aug 9, 2024
61d50f0
Update Changelog.md
ysolomchenko Aug 9, 2024
a1390b4
Update ActivityExtensions.cs by suggestions
ysolomchenko Aug 9, 2024
934895f
Update remarks
ysolomchenko Aug 12, 2024
9e2e73a
Merge branch 'main' into mark-activityextensions.setstatus-obsolete
vishweshbankwar Aug 12, 2024
395d46d
wp
ysolomchenko Aug 14, 2024
0c20670
Merge branch 'mark-activityextensions.setstatus-obsolete' of https://…
ysolomchenko Aug 14, 2024
6c8002a
add status code assertion
ysolomchenko Aug 14, 2024
940d5b5
Merge branch 'main' into mark-activityextensions.setstatus-obsolete
vishweshbankwar Aug 14, 2024
5dca544
apply suggestion in zipkintest
ysolomchenko Aug 16, 2024
2714f35
Merge branch 'mark-activityextensions.setstatus-obsolete' of https://…
ysolomchenko Aug 16, 2024
a04f5d6
update Changelog
ysolomchenko Aug 16, 2024
36bb8e2
Fix changelog
ysolomchenko Aug 16, 2024
8e455e3
remove commented code
ysolomchenko Aug 16, 2024
4a68cf8
Merge branch 'main' into mark-activityextensions.setstatus-obsolete
CodeBlanch Aug 21, 2024
f6a4ebf
Code review.
CodeBlanch Aug 21, 2024
af15a93
Tweaks.
CodeBlanch Aug 21, 2024
9779ed4
Code review.
CodeBlanch Aug 21, 2024
ebfc47b
Test fixes when running against released packages.
CodeBlanch Aug 21, 2024
1eb54ca
Tweaks.
CodeBlanch Aug 21, 2024
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
6 changes: 6 additions & 0 deletions src/OpenTelemetry.Api/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,12 @@ Notes](../../RELEASENOTES.md).

## Unreleased

* Marked `ActivityExtensions.SetStatus` method as `[Obsolete]`.
Users are now encouraged to use the `Activity.SetStatus(ActivityStatusCode)`
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
method from `System.Diagnostics.DiagnosticSource`
for setting the status of an `Activity`.
([#5781](https:/open-telemetry/opentelemetry-dotnet/pull/5781))

* **Breaking change:** CompositeTextMapPropagator.Fields now returns a
unioned set of fields from all combined propagators. Previously this always
returned an empty set.
Expand Down
8 changes: 5 additions & 3 deletions src/OpenTelemetry.Api/Trace/ActivityExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ public static class ActivityExtensions
{
/// <summary>
/// Sets the status of activity execution.
/// Activity class in .NET does not support 'Status'.
/// This extension provides a workaround to store Status as special tags with key name of otel.status_code and otel.status_description.
/// Read more about SetStatus here https:/open-telemetry/opentelemetry-specification/blob/main/specification/trace/api.md#set-status.
/// </summary>
/// <remarks>
ysolomchenko marked this conversation as resolved.
Show resolved Hide resolved
/// This method is obsolete.
ysolomchenko marked this conversation as resolved.
Show resolved Hide resolved
/// For more details see: <see href="https:/open-telemetry/opentelemetry-dotnet/tree/main/src/OpenTelemetry.Api#setting-status" />.
/// </remarks>
/// <param name="activity">Activity instance.</param>
/// <param name="status">Activity execution status.</param>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
[Obsolete("Use Activity.SetStatus(ActivityStatusCode) instead.")]
public static void SetStatus(this Activity activity, Status status)
{
if (activity != null)
Expand Down
4 changes: 4 additions & 0 deletions src/OpenTelemetry.Api/Trace/TelemetrySpan.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ public ActivitySpanId ParentSpanId
/// <param name="value">Status to be set.</param>
public void SetStatus(Status value)
{
#pragma warning disable
// TODO: Remove this when SetStatus is deprecated
this.Activity?.SetStatus(value);
#pragma warning disable
this.Activity?.SetStatus((ActivityStatusCode)value.StatusCode);
Copy link
Member

Choose a reason for hiding this comment

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

not sure why we do this both ways?

Copy link
Member

@vishweshbankwar vishweshbankwar Aug 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is needed for backward compatibility. if we simply remove the older way, we might break code/libraries still relying on that.

}

/// <summary>
Expand Down
2 changes: 2 additions & 0 deletions src/OpenTelemetry/Trace/Processor/ExceptionProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,10 @@ public override void OnEnd(Activity activity)

if (snapshot != pointers)
{
#pragma warning disable
// TODO: Remove this when SetStatus is deprecated
activity.SetStatus(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.

could this be moved to the new SetStatus API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already has a new SetStatus. As @vishweshbankwar mentioned, we need both ways.

#pragma warning disable

// For processors/exporters checking `Status` property.
activity.SetStatus(ActivityStatusCode.Error);
Expand Down
4 changes: 4 additions & 0 deletions test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,11 @@ public void ToOtlpSpanTest()
links: childLinks);

Assert.NotNull(childActivity);
#pragma warning disable
// TODO: Remove this when SetStatus is deprecated
childActivity.SetStatus(Status.Error);
#pragma warning disable
childActivity.SetStatus(ActivityStatusCode.Error);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where the status code is asserted in this test.
I see status description on line 397.

Just curious, if you call SetStatus and provide two different codes, which would win first or last?

Copy link
Member

Choose a reason for hiding this comment

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

There is a separate test for this:

opentelemetry-dotnet/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs

Line 541 in 940d5b5

public void ToOtlpSpanActivityStatusTakesPrecedenceOverStatusTagsWhenActivityStatusCodeIsError()


var childEvents = new List<ActivityEvent> { new("e0"), new("e1", default, new ActivityTagsCollection(attributes)) };
childActivity.AddEvent(childEvents[0]);
Expand Down
3 changes: 3 additions & 0 deletions test/OpenTelemetry.Exporter.Zipkin.Tests/ZipkinExporterTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,10 @@ internal static Activity CreateTestActivity(

if (status.HasValue)
{
#pragma warning disable
activity.SetStatus(status.Value);
#pragma warning enable
activity.SetStatus((ActivityStatusCode)status.Value.StatusCode);
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
}

activity.SetEndTime(endTimestamp);
Expand Down
Loading