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

merged 21 commits into from
Aug 21, 2024

Conversation

ysolomchenko
Copy link
Contributor

Fixes #4755
Design discussion issue #4703

Changes

Mark SetStatus activity extensions method obsolete

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

@github-actions github-actions bot added pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry.Exporter.Zipkin Issues related to OpenTelemetry.Exporter.Zipkin NuGet package labels Aug 9, 2024
@ysolomchenko ysolomchenko marked this pull request as ready for review August 9, 2024 08:55
@ysolomchenko ysolomchenko requested a review from a team August 9, 2024 08:55
cijothomas
cijothomas reviewed Aug 9, 2024
View reviewed changes
src/OpenTelemetry.Api/Trace/TelemetrySpan.cs Outdated
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.

cijothomas
cijothomas reviewed Aug 9, 2024
View reviewed changes
src/OpenTelemetry/Trace/Processor/ExceptionProcessor.cs
@@ -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.

@vishweshbankwar
Copy link
Member

@ysolomchenko - Please take a look at test failures.

TimothyMothra
TimothyMothra reviewed Aug 13, 2024
View reviewed changes
test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs Outdated
Comment on lines 373 to 377
#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()

@codecov Codecov
Copy link

codecov bot commented Aug 16, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.71429% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.35%. Comparing base (6250307) to head (1eb54ca).
Report is 298 commits behind head on main.

Files Patch % Lines
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 84.61% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5781      +/-   ##
==========================================
+ Coverage   83.38%   86.35%   +2.96%     
==========================================
  Files         297      256      -41     
  Lines       12531    11146    -1385     
==========================================
- Hits        10449     9625     -824     
+ Misses       2082     1521     -561
Flag Coverage Δ
unittests ?
unittests-Project-Experimental 85.95% <85.71%> (?)
unittests-Project-Stable 86.27% <85.71%> (?)
unittests-Solution 86.27% <85.71%> (?)
unittests-UnstableCoreLibraries-Experimental 85.67% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/OpenTelemetry.Api/Trace/TelemetrySpan.cs 88.40% <100.00%> (ø)
...penTelemetry/Trace/Processor/ExceptionProcessor.cs 100.00% <ø> (ø)
src/OpenTelemetry.Api/Trace/ActivityExtensions.cs 90.32% <84.61%> (-5.14%) ⬇️

... and 208 files with indirect coverage changes

vishweshbankwar
vishweshbankwar approved these changes Aug 19, 2024
View reviewed changes
Copy link
Member

@vishweshbankwar vishweshbankwar left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the pkg:OpenTelemetry.Shims.OpenTracing Issues related to OpenTelemetry.Shims.OpenTracing NuGet package label Aug 21, 2024
@CodeBlanch CodeBlanch changed the title Mark ActivityExtensions.SetStatus obsolete [api] Mark SetStatus & GetStatus ActivityExtensions obsolete Aug 21, 2024
@CodeBlanch CodeBlanch merged commit 1b3f189 into open-telemetry:main Aug 21, 2024
42 checks passed
This was referenced Oct 1, 2024
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cijothomas cijothomas cijothomas left review comments

@CodeBlanch CodeBlanch CodeBlanch left review comments

@TimothyMothra TimothyMothra TimothyMothra left review comments

@lachmatt lachmatt lachmatt left review comments

@vishweshbankwar vishweshbankwar vishweshbankwar approved these changes

Assignees
No one assigned
Labels
pkg:OpenTelemetry.Api Issues related to OpenTelemetry.Api NuGet package pkg:OpenTelemetry.Exporter.OpenTelemetryProtocol Issues related to OpenTelemetry.Exporter.OpenTelemetryProtocol NuGet package pkg:OpenTelemetry.Exporter.Zipkin Issues related to OpenTelemetry.Exporter.Zipkin NuGet package pkg:OpenTelemetry.Shims.OpenTracing Issues related to OpenTelemetry.Shims.OpenTracing NuGet package pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mark ActivityExtensions.SetStatus obsolete
6 participants
@ysolomchenko @vishweshbankwar @cijothomas @CodeBlanch @TimothyMothra @lachmatt