-
Notifications
You must be signed in to change notification settings - Fork 302
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.Extensions.AWSXRay] to [OpenTelemetry.Extensions.AWS] #1232
Conversation
@srprash, @atshaw43, @Kielek and @Oberon00: I would be very thankful for your review and comments. |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1232 +/- ##
==========================================
+ Coverage 72.96% 73.61% +0.65%
==========================================
Files 265 265
Lines 9309 9346 +37
==========================================
+ Hits 6792 6880 +88
+ Misses 2517 2466 -51
|
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.
I have pushed some minor changes directly to your branch.
LGTM.
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 should be reviewed by the package owners @srprash @atshaw43.
LGTM, but:
After this is released we probably need to update all the pseudo-internal dependencies as well, e.g. https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/Instrumentation.AWSLambda-1.1.0-beta.2/src/OpenTelemetry.Instrumentation.AWSLambda/OpenTelemetry.Instrumentation.AWSLambda.csproj#L15, otherwise anybody using a package depending on the old package cannot use the new package. In general renaming a package that has quite a few dependents seems to be asking for trouble. I wonder if this is worth it, but this should be decided by maintainers or package owners. I have no strong opinion.
labels: comp:contrib.extensions.awsxray | ||
name: OpenTelemetry.Extensions.AWS | ||
about: Issue with OpenTelemetry.Extensions.AWS | ||
labels: comp:extensions.aws |
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 this is merged, don't forget to rename the label on GitHub as well, @open-telemetry/dotnet-contrib-maintainers
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
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.
Make sure not to merge, there is a mistake here: The goal was to remove the "Contrib" part from the name, but this renames AWSXray to AWS which would then ultimately lead to conflicts.
Or maybe it's not a mistake, I'm a bit confused now. I'm pretty sure we want to remove "contrib" but why do we want to remove "xray"? Will this then not be easy to confuse with the other "AWS" package (Extensions vs Instrumentation)? |
@Oberon00: I think renaming to OpenTelemetry.Extensions.AWS should be fine as it will no conflict with OpenTelemetry.Instrumentation.AWS. Also it more or less reflects what is this package for. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Name of the package OpenTelemetry.Contrib.Extensions.AWSXRay is not consistent with current naming conventions.
Package has been renamed to OpenTelemetry.Extensions.AWS. Namespaces, etc have been aligned accordingly.
This PR is result of discussions in the other PR: #1224 (comment)
EDIT: Fixes: #1066