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 package: [OpenTelemetry.Contrib.Instrumentation.AWS] to [OpenTelemetry.Instrumentation.AWS] #1275

Merged

Conversation

rypdal
Copy link
Contributor

@rypdal rypdal commented Jul 21, 2023

Name of the package OpenTelemetry.Contrib.Instrumentation.AWS is not consistent with current naming conventions and Contrib suffix should be removed.

Package project and all related artefacts like namespaces, etc should be changed to OpenTelemetry.Instrumentation.AWS

This PR is the result of discussions in the other PR: #1224 (comment)

@rypdal rypdal requested a review from a team July 21, 2023 14:07
@rypdal
Copy link
Contributor Author

rypdal commented Jul 21, 2023

FYI: @Oberon00 and @Kielek

@Oberon00
Copy link
Member

I just noticed that the AWS packages are also the only ones that break the capitalization rule at https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions#capitalization-rules-for-identifiers which states that for acronyms longer than two letters, only the first one should be uppercased. So the namespace & package name ought to be Aws instead of AWS.
However, I wonder if this is worth the disruption (if we also rename the AwsLambda/AWSLambda package) or inconsistency (if we don't).

@utpilla utpilla added the comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS label Jul 21, 2023
@rypdal
Copy link
Contributor Author

rypdal commented Jul 24, 2023

I just noticed that the AWS packages are also the only ones that break the capitalization rule at https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/capitalization-conventions#capitalization-rules-for-identifiers which states that for acronyms longer than two letters, only the first one should be uppercased. So the namespace & package name ought to be Aws instead of AWS. However, I wonder if this is worth the disruption (if we also rename the AwsLambda/AWSLambda package) or inconsistency (if we don't).

I think it's a subject of different PR where we should include AWSLambda package too. The current renaming follows initial rules introduced in AWS projects.

…elemetry.Instrumentation.AWS]: removed redundant project file
…elemetry.Instrumentation.AWS]:

- inverted changes introduced by auto-refactoring (only renaming related changes should be kept)
- keep the namespace name for TracerProviderBuilderExtensions
…elemetry.Instrumentation.AWS]: reverted one more line appeared after auto-refactoring
…elemetry.Instrumentation.AWS]: updated CHANGELOG
…elemetry.Instrumentation.AWS]: moved the latest unreleased change record to the top
@rypdal
Copy link
Contributor Author

rypdal commented Jul 24, 2023

Seems failed tests appeared after the merge from main.

@rypdal
Copy link
Contributor Author

rypdal commented Jul 26, 2023

@srprash , @Kielek , @Oberon00: could this PR be merged ?

@Kielek
Copy link
Contributor

Kielek commented Jul 26, 2023

As soon as we find a way to fix this step: Build / build-test (windows-latest, net462) (pull_request).

In general it is passing in different PRs.

@Kielek
Copy link
Contributor

Kielek commented Jul 27, 2023

Merging per comment #875 (comment)

@Kielek Kielek merged commit bcb632d into open-telemetry:main Jul 27, 2023
26 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.aws Things related to OpenTelemetry.Instrumentation.AWS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants