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

[Instrumentation.Process] Cleanup public APIs. #973

Merged
merged 15 commits into from
Feb 8, 2023

Conversation

Yun-Ting
Copy link
Contributor

@Yun-Ting Yun-Ting commented Feb 6, 2023

Changes

Cleanup public APIs.

  • Appropriate CHANGELOG.md updated for non-trivial changes

@Yun-Ting Yun-Ting requested a review from a team February 6, 2023 22:23
/// </summary>
/// <param name="builder"><see cref="MeterProviderBuilder"/> being configured.</param>
/// <param name="configure">Runtime metrics options.</param>
/// <param name="configure">Process metrics options.</param>
Copy link
Member

Choose a reason for hiding this comment

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

this method can be removed for now as there is nothing to configure in ProcessInstrumentaionOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

Copy link
Contributor

Choose a reason for hiding this comment

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

The option class seems to be also not needed.

Copy link
Member

Choose a reason for hiding this comment

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

okay to leave it empty or keep internal. I think it was put as placeholder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated to use internal

cijothomas
cijothomas previously approved these changes Feb 6, 2023
@codecov
Copy link

codecov bot commented Feb 6, 2023

Codecov Report

Merging #973 (4b7d5ef) into main (2cf37da) will decrease coverage by 0.03%.
The diff coverage is 92.30%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #973      +/-   ##
==========================================
- Coverage   68.37%   68.34%   -0.03%     
==========================================
  Files         183      183              
  Lines        7026     7023       -3     
==========================================
- Hits         4804     4800       -4     
- Misses       2222     2223       +1     
Impacted Files Coverage Δ
...entation.Process/MeterProviderBuilderExtensions.cs 100.00% <ø> (ø)
...umentation.Owin/TracerProviderBuilderExtensions.cs 83.33% <50.00%> (-16.67%) ⬇️
...etry.Instrumentation.AWSLambda/AWSLambdaWrapper.cs 94.54% <100.00%> (-0.20%) ⬇️
...ion.AWSLambda/Implementation/AWSLambdaHttpUtils.cs 100.00% <100.00%> (ø)

@Kielek
Copy link
Contributor

Kielek commented Feb 7, 2023

Please update changelog. It might be breaking change for part of the users.

@cijothomas cijothomas dismissed their stale review February 7, 2023 16:33

need changelog update as this PR is no longer a nit comment fix

@Kielek Kielek changed the title [Instrumentation.Process] nits: fix comments [Instrumentation.Process] Cleanup public API Feb 7, 2023
@Yun-Ting Yun-Ting changed the title [Instrumentation.Process] Cleanup public API [Instrumentation.Process] Cleanup public APIs. Feb 7, 2023
@cijothomas cijothomas merged commit a2960dc into open-telemetry:main Feb 8, 2023
@Yun-Ting Yun-Ting deleted the yunl/nit branch February 8, 2023 19:34
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