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 OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetector.Container #881

Closed
wants to merge 2 commits into from

Conversation

Kielek
Copy link
Contributor

@Kielek Kielek commented Jan 10, 2023

Fixes #869.

Changes

Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetector.Container.
Label in repository changed from comp:extensions.docker to comp:resourcedetector.container

From changelog

For significant contributions please make sure you have completed the following items:

@Kielek Kielek added the comp:resources.container Things related to OpenTelemetry.Resources.Container label Jan 10, 2023
@Kielek Kielek marked this pull request as ready for review January 10, 2023 07:39
@Kielek Kielek requested a review from a team January 10, 2023 07:39
@Kielek
Copy link
Contributor Author

Kielek commented Jan 10, 2023

@iskiselev could you please chek?

@codecov
Copy link

codecov bot commented Jan 10, 2023

Codecov Report

Merging #881 (40cc93a) into main (70f9f0f) will increase coverage by 0.41%.
The diff coverage is 79.31%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #881      +/-   ##
==========================================
+ Coverage   79.07%   79.48%   +0.41%     
==========================================
  Files         182      182              
  Lines        5614     5629      +15     
==========================================
+ Hits         4439     4474      +35     
+ Misses       1175     1155      -20     
Impacted Files Coverage Δ
...r.Stackdriver/Implementation/ActivityExtensions.cs 75.00% <ø> (ø)
...ons/Internal/ActivityEventAttachingLogProcessor.cs 93.33% <0.00%> (ø)
...on.EventCounters/MeterProviderBuilderExtensions.cs 100.00% <ø> (ø)
...elemetry.Instrumentation.Process/ProcessMetrics.cs 100.00% <ø> (ø)
....ResourceDetector.Container/Utils/EncodingUtils.cs 100.00% <ø> (ø)
...rceDetector.Container/ContainerResourceDetector.cs 82.60% <76.19%> (ø)
...ry.Extensions/Internal/DefaultLogStateConverter.cs 87.50% <100.00%> (ø)
...nsions/Logs/LogToActivityEventConversionOptions.cs 100.00% <100.00%> (ø)
...try.Extensions/Trace/AutoFlushActivityProcessor.cs 81.48% <100.00%> (ø)
....Container/ContainerResourceDetectorEventSource.cs 8.33% <100.00%> (ø)
... and 3 more

@iskiselev
Copy link
Contributor

Looks good to me. Thank you, @Kielek. I've raised small concern regards including ResourceDetector in this package name in #869. Let's confirm that we really want include it for this package (changing it from hosting-technology helper to only be resource detector) in the issue before merge.
We should not close #869 ticket until we release package with updated name and mark old package as deprecated in NuGet with link to updated (https://learn.microsoft.com/en-us/nuget/nuget-org/deprecate-packages).

Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Docker", "src\OpenTelemetry.Extensions.Docker\OpenTelemetry.Extensions.Docker.csproj", "{498A6808-C0DF-441F-A764-51A3BC4B8FC5}"
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.ResourceDetector.Container", "src\OpenTelemetry.ResourceDetector.Container\OpenTelemetry.ResourceDetector.Container.csproj", "{498A6808-C0DF-441F-A764-51A3BC4B8FC5}"
Copy link
Member

Choose a reason for hiding this comment

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

@Kielek @iskiselev We discussed this name change in the SIG meeting yesterday (1/10).

While we're pretty confident this is a better name for this package, I'd like to hold on merging it for just a bit.

I'm compiling a list of packages whose names I believe should also change. I'd like to go through that exercise first before making a final decision on this package. I'd hate to change this package only to decide later we want to change it again.

Question: What do the two of you think about releasing the package once more under the old name OpenTelemetry.Extensions.Docker so we can get the work from #839 released independent of the name change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@alanwest , I can do release with #839 functionality. I'd probably will use 1.1.0-beta as a version (so it will still be considered pre-release and will not affect on shipped/unshipped API).
As soon as we will select name, we will do 1.1.0 release (without-beta suffix).

Copy link
Contributor Author

@Kielek Kielek Jan 11, 2023

Choose a reason for hiding this comment

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

@alanwest , It is fine for me to release it without renaming.
If you want to wait a bit longer (more than a week o two), I would consider to close the PR for now and reopen when needed. Feel free to do it if you agree.

iskiselev added a commit to iskiselev/opentelemetry-dotnet-contrib that referenced this pull request Jan 12, 2023
* Changes
Update CHANGELOG for 1.1.0-beta.1 release, that will include CGroup V2 support (open-telemetry#839, open-telemetry#693). It still have `-beta.1` suffix to make it prerelease until decision on package name will be done (open-telemetry#881, open-telemetry#869)
@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Jan 19, 2023
@github-actions
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Jan 27, 2023
@Kielek Kielek deleted the rename-docker-package branch January 27, 2023 05:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:resources.container Things related to OpenTelemetry.Resources.Container Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetectors.Container
3 participants