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

[Part2] Support Activity Status and status description in Jaeger Exporter. #3073

Merged
merged 9 commits into from
Mar 24, 2022

Conversation

Yun-Ting
Copy link
Contributor

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

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

Added support for Activity.Status/Description in Jaeger.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 if it was 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.

In order to support Activity Status takes precedence over Activity Tags, the logic that were in ProcessJaegerTag were extracted to be inside ForEach() method of TagEnumerationState.

Changes

Please provide a brief description of the changes here.

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

@Yun-Ting Yun-Ting marked this pull request as ready for review March 23, 2022 01:29
@Yun-Ting Yun-Ting requested a review from a team March 23, 2022 01:29
@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #3073 (a35fff7) into main (f531391) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3073      +/-   ##
==========================================
+ Coverage   84.75%   84.77%   +0.02%     
==========================================
  Files         259      259              
  Lines        9142     9170      +28     
==========================================
+ Hits         7748     7774      +26     
- Misses       1394     1396       +2     
Impacted Files Coverage Δ
....Jaeger/Implementation/JaegerActivityExtensions.cs 96.40% <100.00%> (+1.44%) ⬆️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 73.52% <0.00%> (-8.83%) ⬇️

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.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

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.

3 participants