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.

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

Replace ActivityExtensions.SetStatus by Activity.SetStatus #2358

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Dec 4, 2024

Fixes #2321

Changes

Replace ActivityExtensions.SetStatus by Activity.SetStatus
Following tags will be not added to spans when the exception is recorder:
otel.status_code and otel.status_description. Both values are handled natively.

Changes affects only pre-released instrumentation packages. It should be safe to merge as is.

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 comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda comp:instrumentation.confluentkafka Things related to OpenTelemetry.Instrumentation.ConfluentKafka comp:instrumentation.entityframeworkcore Things related to OpenTelemetry.Instrumentation.EntityFrameworkCore comp:instrumentation.owin Things related to OpenTelemetry.Instrumentation.Owin comp:instrumentation.quartz Things related to OpenTelemetry.Instrumentation.Quartz comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf labels Dec 4, 2024
Copy link

codecov bot commented Dec 4, 2024

Codecov Report

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

Project coverage is 65.30%. Comparing base (71655ce) to head (085afcb).
Report is 624 commits behind head on main.

Files with missing lines Patch % Lines
...luentKafka/OpenTelemetryConsumeResultExtensions.cs 0.00% 1 Missing ⚠️
...mplementation/TelemetryDispatchMessageInspector.cs 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2358      +/-   ##
==========================================
- Coverage   73.91%   65.30%   -8.61%     
==========================================
  Files         267       98     -169     
  Lines        9615     2672    -6943     
==========================================
- Hits         7107     1745    -5362     
+ Misses       2508      927    -1581     
Flag Coverage Δ
unittests-Instrumentation.AWS 86.27% <100.00%> (?)
unittests-Instrumentation.ConfluentKafka 14.37% <0.00%> (?)
unittests-Instrumentation.EntityFrameworkCore 57.06% <100.00%> (?)
unittests-Instrumentation.Owin 88.12% <100.00%> (?)
unittests-Instrumentation.Quartz 78.76% <100.00%> (?)
unittests-Instrumentation.Wcf 78.57% <50.00%> (?)

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

Files with missing lines Coverage Δ
...ntation.AWS/Implementation/Tracing/AWSTraceSpan.cs 55.26% <100.00%> (ø)
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs 94.82% <100.00%> (ø)
...mplementation/EntityFrameworkDiagnosticListener.cs 53.03% <100.00%> (+0.64%) ⬆️
...ation.Owin/Implementation/DiagnosticsMiddleware.cs 94.79% <100.00%> (+2.93%) ⬆️
....Quartz/Implementation/QuartzDiagnosticListener.cs 82.60% <100.00%> (-4.49%) ⬇️
...Wcf/Implementation/ClientChannelInstrumentation.cs 93.50% <100.00%> (-1.30%) ⬇️
...luentKafka/OpenTelemetryConsumeResultExtensions.cs 0.00% <0.00%> (ø)
...mplementation/TelemetryDispatchMessageInspector.cs 88.88% <0.00%> (ø)

... and 290 files with indirect coverage changes

@Kielek Kielek force-pushed the setstatus-obsolete branch from 1b431ff to 894bc11 Compare December 4, 2024 08:22
@Kielek Kielek marked this pull request as ready for review December 4, 2024 08:27
@Kielek Kielek requested a review from a team as a code owner December 4, 2024 08:27
Copy link
Contributor

@g7ed6e g7ed6e left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 5 to 7
* The following tags are no longer added to spans when an exception is recorded:
`otel.status_code` and `otel.status_description`. Both values are handled natively.
([#2358](https://github.com/open-telemetry/opentelemetry-dotnet-contrib/pull/2358))
Copy link
Member

Choose a reason for hiding this comment

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

We may want to change this description. Suggestion something like:

  • Trace instrumentation will now call the Activity.SetStatus API instead of the deprecated OpenTelemetry API package extension when setting span status. For details see: Setting Status.

Why?

The way it is currently written, a user might think otel.status_code and otel.status_description will no longer be present when the data hits the backend. But that depends largely on their exporter. For example Zipkin will add those based on the final Activity.Status/StatusDescription: https://github.com/open-telemetry/opentelemetry-dotnet/blob/0c775e58fa8eb55468f321d5612b527ed80afb9f/src/OpenTelemetry.Exporter.Zipkin/Implementation/ZipkinActivityConversionExtensions.cs#L51-L62

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

Suggest changing CHANGELOGs but fundamentally LGTM

@github-actions github-actions bot requested a review from CodeBlanch December 6, 2024 06:05
@Kielek Kielek merged commit 9d50640 into open-telemetry:main Dec 6, 2024
77 checks passed
@Kielek Kielek deleted the setstatus-obsolete branch December 6, 2024 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS comp:instrumentation.awslambda Things related to OpenTelemetry.Instrumentation.AWSLambda comp:instrumentation.confluentkafka Things related to OpenTelemetry.Instrumentation.ConfluentKafka comp:instrumentation.entityframeworkcore Things related to OpenTelemetry.Instrumentation.EntityFrameworkCore comp:instrumentation.owin Things related to OpenTelemetry.Instrumentation.Owin comp:instrumentation.quartz Things related to OpenTelemetry.Instrumentation.Quartz comp:instrumentation.wcf Things related to OpenTelemetry.Instrumentation.Wcf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace ActivityExtensions.SetStatus by Activity.SetStatus(.WithDescription)
10 participants