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

[Part3] Support Activity Status and status description in OTLP Exporter. #3100

Merged
merged 18 commits into from
Mar 29, 2022

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Mar 28, 2022

Changes

Fixes parts of #2569.
Related to: #3003 and #3073
Background : Please review Console.Exporter where the same logic were exercised: #3061.

Added support for Activity.Status/Description in OTLP.Exporter.
System.Diagnostic.DiagnosticSource version 6.0.0 introduced native support for storing status/description in the Activity itself.

This PR modified the exporter to retrieve status from the newly added fields (Activity.Status and Activity StatusDescription) if they were set.
To maintain backward compatibility, the control flow would fell back to retrieve status from the activity.Tags if the native status/description were not set.

Prior to this PR if Status=Unset then the exporter would set both the status and description. This PR skipped setting status and statusDescription if the Status is Unset. Additionally, added statusDescription only if the Status=Error.

For significant contributions please make sure you have completed the following items:

  • Appropriate CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@codecov
Copy link

codecov bot commented Mar 28, 2022

Codecov Report

Merging #3100 (9d1e9c9) into main (bea9b60) will decrease coverage by 0.15%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3100      +/-   ##
==========================================
- Coverage   84.97%   84.82%   -0.16%     
==========================================
  Files         259      259              
  Lines        9179     9185       +6     
==========================================
- Hits         7800     7791       -9     
- Misses       1379     1394      +15     
Impacted Files Coverage Δ
...metryProtocol/Implementation/ActivityExtensions.cs 88.73% <100.00%> (-1.08%) ⬇️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 50.00% <0.00%> (-28.58%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 59.09% <0.00%> (-18.19%) ⬇️
...mentation/ExportClient/BaseOtlpGrpcExportClient.cs 61.11% <0.00%> (-5.56%) ⬇️
...nTelemetry/Internal/OpenTelemetrySdkEventSource.cs 71.05% <0.00%> (-2.64%) ⬇️

@Yun-Ting Yun-Ting marked this pull request as ready for review March 28, 2022 21:25
@Yun-Ting Yun-Ting requested a review from a team March 28, 2022 21:25
Copy link
Member

@alanwest alanwest left a comment

Choose a reason for hiding this comment

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

LGTM. I'd be ok with #3100 (comment) being a follow up if you'd prefer.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM with some minor suggestions.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

@cijothomas cijothomas merged commit add7eee into open-telemetry:main Mar 29, 2022
@Yun-Ting Yun-Ting deleted the yunl/otlpStatus branch March 29, 2022 22:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants