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

Updated AddLegacyActivity method name to AddLegacySource #1860

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Mar 1, 2021

Changes

  • Updated AddLegacyActivity to AddLegacySource
  • CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@utpilla utpilla requested a review from a team March 1, 2021 07:33
@codecov
Copy link

codecov bot commented Mar 1, 2021

Codecov Report

Merging #1860 (ed4f9cf) into main (cb066cb) will decrease coverage by 0.43%.
The diff coverage is 78.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
- Coverage   83.77%   83.34%   -0.44%     
==========================================
  Files         187      188       +1     
  Lines        5967     6130     +163     
==========================================
+ Hits         4999     5109     +110     
- Misses        968     1021      +53     
Impacted Files Coverage Δ
src/OpenTelemetry/Sdk.cs 100.00% <ø> (ø)
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 26.82% <20.54%> (-44.60%) ⬇️
src/OpenTelemetry/Logs/LogRecord.cs 82.50% <77.77%> (-5.00%) ⬇️
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs 77.55% <88.23%> (+7.09%) ⬆️
src/OpenTelemetry/Trace/ExceptionProcessor.cs 90.90% <90.90%> (ø)
...rc/OpenTelemetry/Trace/TracerProviderBuilderSdk.cs 89.83% <91.66%> (-0.92%) ⬇️
src/OpenTelemetry/Trace/TracerProviderSdk.cs 95.16% <98.64%> (+4.38%) ⬆️
...metryProtocol/Implementation/ActivityExtensions.cs 86.89% <100.00%> (+0.05%) ⬆️
...ry.Instrumentation.AspNet/AspNetInstrumentation.cs 100.00% <100.00%> (ø)
...umentation.AspNet/Implementation/HttpInListener.cs 89.10% <100.00%> (ø)
... and 23 more

@@ -132,9 +132,9 @@ internal TracerProviderBuilder AddProcessor(BaseProcessor<Activity> processor)
/// <summary>
Copy link
Member

Choose a reason for hiding this comment

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

I think the code comments should use a plural tense. Here's an alternative:

        /// <summary>
        /// Adds a listener for <see cref="Activity"/> objects created with the given operation name to the <see cref="TracerProviderBuilder"/>.
        /// </summary>
        /// <remarks>
        /// This is provided to capture legacy <see cref="Activity"/> objects creared without using the <see cref="ActivitySource"/> API.
        /// </remarks>
        /// <param name="tracerProviderBuilder"><see cref="TracerProviderBuilder"/> instance.</param>
        /// <param name="operationName">Operation name of the <see cref="Activity"/> objects to capture.</param>
        /// <returns>Returns <see cref="TracerProviderBuilder"/> for chaining.</returns>
        public static TracerProviderBuilder AddLegacyActivity(this TracerProviderBuilder tracerProviderBuilder, string operationName)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the code comments.

@@ -9,16 +9,20 @@ please check the latest changes

## Unreleased

* Added `ForceFlush` to `TracerProvider`. ([#1837](https://github.com/open-telemetry/opentelemetry-dotnet/pull/1837))
* Updated the method name of `AddLegacyActivity` to `AddLegacySource`
Copy link
Member

Choose a reason for hiding this comment

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

no need of this. as this was never released.

@cijothomas cijothomas merged commit fa8d00c into open-telemetry:main Mar 9, 2021
@utpilla utpilla deleted the utpilla/Update-AddLegacyActivity-To-AddLegacySource branch July 30, 2021 00:18
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