-
Notifications
You must be signed in to change notification settings - Fork 305
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
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.
|
1b431ff
to
894bc11
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* 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.
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
There was a problem hiding this 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
Co-authored-by: Mikel Blanchard <[email protected]>
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
andotel.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.md
files updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)