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

Rename Diag/LoggerOptions to DiagLoggerOptions #3641

Merged
merged 6 commits into from
Mar 1, 2023

Conversation

altinokdarici
Copy link
Contributor

Which problem is this PR solving?

Conflicting type name LoggerOptions in API and experimental logs API

Fixes #3640

Short description of the changes

Renaming LoggerOptions to DiagLoggerOptions

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

N/A

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@altinokdarici altinokdarici marked this pull request as ready for review February 28, 2023 09:09
@altinokdarici altinokdarici requested a review from a team as a code owner February 28, 2023 09:09
@codecov
Copy link

codecov bot commented Feb 28, 2023

Codecov Report

Merging #3641 (f72bde9) into main (0d373bd) will increase coverage by 0.19%.
The diff coverage is n/a.

❗ Current head f72bde9 differs from pull request most recent head 54492a2. Consider uploading reports for the commit 54492a2 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3641      +/-   ##
==========================================
+ Coverage   93.52%   93.72%   +0.19%     
==========================================
  Files         271      274       +3     
  Lines        7496     8058     +562     
  Branches     1521     1670     +149     
==========================================
+ Hits         7011     7552     +541     
- Misses        485      506      +21     
Impacted Files Coverage Δ
api/src/diag/types.ts 100.00% <ø> (ø)
...emetry-core/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...es/opentelemetry-instrumentation-http/src/utils.ts 99.18% <0.00%> (ø)
...ges/opentelemetry-instrumentation-http/src/http.ts 94.26% <0.00%> (ø)
...y-instrumentation-http/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

Copy link
Member

@pichlermarc pichlermarc left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this. 🙂
Should be safe to change, looks like this type is indeed not exported. 🙂

@altinokdarici
Copy link
Contributor Author

@Flarna, @pichlermarc thanks for approval. How can I get this PR merged?

@pichlermarc
Copy link
Member

We usually keep it open for some time (1-2 days from opening the PR) so everyone can have a look, then we'll merge it if approved. No actions are necessary except for the occasional branch update. 🙂

@dyladan dyladan added the pkg:api label Mar 1, 2023
@dyladan
Copy link
Member

dyladan commented Mar 1, 2023

There's been enough reviews I think to merge this without waiting

@dyladan
Copy link
Member

dyladan commented Mar 1, 2023

One question is why is the type not exposed? It looks like in the diag API the type isn't used where it should be in src/api/diag.ts DiagAPI#setLogger

@Flarna
Copy link
Member

Flarna commented Mar 1, 2023

see #3639 where I notices the name conflict resulting in #3640 and finally this PR.

@dyladan dyladan merged commit 31dd7b7 into open-telemetry:main Mar 1, 2023
@altinokdarici altinokdarici deleted the rename-logger-options branch March 1, 2023 23:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conflicting type name in API and experimental logs API
7 participants