Replace ActivityExtensions.SetStatus by Activity.SetStatus - #2358
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| * 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)) |
There was a problem hiding this comment.
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
CodeBlanch
left a comment
There was a problem hiding this comment.
Suggest changing CHANGELOGs but fundamentally LGTM
Co-authored-by: Mikel Blanchard <mblanchard@macrosssoftware.com>
Fixes #2321
Changes
Replace ActivityExtensions.SetStatus by Activity.SetStatus
Following tags will be not added to spans when the exception is recorder:
otel.status_codeandotel.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
CHANGELOG.mdfiles updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)