-
Notifications
You must be signed in to change notification settings - Fork 773
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
Move AddLegacySource to API #2019
Move AddLegacySource to API #2019
Conversation
@@ -14,6 +14,7 @@ | |||
// limitations under the License. | |||
// </copyright> | |||
using System; | |||
using System.Diagnostics; |
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.
Without this, the project fails to build due to the XML comment below on AddLegacySource. Wondering if this is enough of a drain to prefer instead rewriting the below comment?
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.
if you want, you could move this to the comment. So, System.Diagnostics.Activity
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.
This shouldn't affect the runtime performance of the application, right? We should add that using directive so that the method documentation is written with brevity.
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.
Dont think there is any issue here. My personal preference is to not add the using Sys.Diag, and instead use the full name in the doc comment. It'll make using look cleaner.
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.
Merging without this - you can address this in a separate PR.
Codecov Report
@@ Coverage Diff @@
## main #2019 +/- ##
=======================================
Coverage 83.87% 83.88%
=======================================
Files 192 192
Lines 6184 6181 -3
=======================================
- Hits 5187 5185 -2
+ Misses 997 996 -1
|
@@ -9,6 +9,10 @@ please check the latest changes | |||
|
|||
## Unreleased | |||
|
|||
* `AddLegacySource()` moved out of `TracerProviderBuilderExtensions` and into | |||
public API |
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.
it was and still is part of public API. Might be a good idea to reword this.
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 can adjust this separate PR.
Fixes #1983.
Changes
AddLegacySource(string operationname), used for legacy activities without the use of ActivitySource, should be in our public API.
No changes to the functionality of these methods.
CHANGELOG.md
updated for non-trivial changes