[Exporter.Console] Add support for ActivitySource.Version#5472
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5472 +/- ##
==========================================
+ Coverage 83.38% 85.51% +2.13%
==========================================
Files 297 290 -7
Lines 12531 12608 +77
==========================================
+ Hits 10449 10782 +333
+ Misses 2082 1826 -256
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| this.WriteLine($"Activity.ActivitySourceName: {activity.Source.Name}"); | ||
| if (!string.IsNullOrEmpty(activity.Source.Version)) | ||
| { | ||
| this.WriteLine($"Activity.ActivitySourceVersion: {activity.Source.Version}"); |
There was a problem hiding this comment.
Why do we want Activity.ActivitySourceVersion instead of Activity.Source.Version?
There was a problem hiding this comment.
Same convention as for ActivitySourceName
There was a problem hiding this comment.
It maybe beneficial to use ActivitySourceVersion(aka Tracer version) to make it easier to realize that ActivitySource and Tracer are the same concept.
AND/OR use InstrumentationScope wording?
There was a problem hiding this comment.
I am less concerned about wording as it is mostly for debugging purposes. It should be fine to have it this value in any form.
I can make additional changes in this PR on follow up PR if we have agreement. It will be great to include this fix on 1.8.0 release so I would like to avoid long discussion.
There was a problem hiding this comment.
This is a non-blocking comment. We do have another task to revisit console output format, so any additional improvements can be addressed as part of that.
6 participants
Fixes #
Design discussion issue #
Changes
[Exporter.Console] Add support for ActivitySource.Version
Merge requirement checklist
[ ] Unit tests added/updatedNo tests for similar functionalities.CHANGELOG.mdfiles updated for non-trivial changes[ ] Changes in public API reviewed (if applicable)