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.ResourceDetectors.Container #869

Closed
Kielek opened this issue Jan 5, 2023 · 6 comments · Fixed by #1123
Closed
Assignees
Labels
comp:resources.container Things related to OpenTelemetry.Resources.Container

Comments

@Kielek
Copy link
Contributor

Kielek commented Jan 5, 2023

@iskiselev based on the PR description package will support not only Docker, but also everything based on container id. I suppose the package name should be changed from OpenTelemetry.Extensions.Docker to OpenTelemetry.Extensions.Container. Also all public API should be adjusted. I would keep it as separate PR.

@Kielek, the name can be changed, as more generic name OpenTelemetry.Extensions.Container - but we already have first release of it at https://www.nuget.org/packages/OpenTelemetry.Extensions.Docker - we will need to deprecate old package in NuGet and add link to new package name there. I don't know how to do it, but hope it won't be too hard. We can create separate issue and discuss it there. Looks like nodejs have already renamed it to container.

Originally posted by @iskiselev in #839 (comment)

@Kielek Kielek added the comp:resources.container Things related to OpenTelemetry.Resources.Container label Jan 5, 2023
@alanwest
Copy link
Member

alanwest commented Jan 5, 2023

If we're renaming this package now, this is probably a good time to consider the general proposal I have here open-telemetry/opentelemetry-dotnet#3469 for revising our naming strategy across our packages.

Namely, I question the overuse of the word Extensions in our package names. We use it for so many different kinds of packages that it effectively has no meaning.

My suggestion for this package was to consider something more descriptive of what is in it: OpenTelemetry.ResourceDetectors.Container. We do this for other SDK extensions like exporters e.g. OpenTelemetry.Exporter.Jaeger. Seems fitting to include the kind of SDK extension in the package name itself.

@utpilla
Copy link
Contributor

utpilla commented Jan 5, 2023

If we're renaming this package now, this is probably a good time to consider the general proposal I have here open-telemetry/opentelemetry-dotnet#3469 for revising our naming strategy across our packages.

Namely, I question the overuse of the word Extensions in our package names. We use it for so many different kinds of packages that it effectively has no meaning.

I agree. If we have the opportunity to make the package names more descriptive, we should.

My suggestion for this package was to consider something more descriptive of what is in it: OpenTelemetry.ResourceDetectors.Container.

We should also consider standardizing the usage of singular vs plural nouns in the package names. Do we want to name is ResourceDetector or ResourceDectectors. We have always used plural for extensions even if the project would have only one extension method.

@Kielek Kielek changed the title Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.Extensions.Container Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetectors.Container Jan 9, 2023
@alanwest
Copy link
Member

alanwest commented Jan 9, 2023

We should also consider standardizing the usage of singular vs plural nouns in the package names. Do we want to name is ResourceDetector or ResourceDectectors

Yep, good call. My vote singular as that keeps things consistent with our already stable packages (e.g., OpenTelemetry.Exporter.*). So in this case: OpenTelemetry.ResourceDetector.Container

@Kielek Kielek changed the title Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetectors.Container Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetector.Container Jan 9, 2023
@Kielek Kielek self-assigned this Jan 10, 2023
@iskiselev
Copy link
Contributor

I have one small concern. Originally we took a sample from OpenTelemetry.Contrib.Extensions.AWSXRay, which have both trace propagator and several resource. With the name OpenTelemetry.ResourceDetector.Container we will limit ourself with only resource detector, versus set of helpers for those, who use Containers.
I can't say now, if anything beyond ResourceDetector will be used, but would like to discuss it.
There is some difference in additional exported, instrumentation of some framework and utility set to support some specific hosting site. That package is the last category, and was not limited by ResourceDetector (though now only ResourceDetector included).

@alanwest
Copy link
Member

I can't say now, if anything beyond ResourceDetector will be used, but would like to discuss it.

A few others have asked this same question as well. It's a good reason not to jump into a name change too quickly.

Here's some ideas

  1. Decide that, in general, we will aim to ship small packages often containing only one SDK extension (i.e., an exporter, a resource detector, a span processor, etc.). If they contain multiple SDK extensions, they will be of the same type. If later we wanted to ship a different type of thing, we'd ship it in a different package e.g., OpenTelemetry.Instrumentation.Container. Later, if this ends up resulting in a large number of related packages, we could consider shipping a meta package - e.g. OpenTelemetry.Container.
  2. Alternatively, if we know early on that it is highly likely for something to require a lot of different kind of SDK extensions then maybe we just adopt a name like OpenTelemetry.Container or OpenTelemetry.ContainerExtensions from the beginning.

Personally, I'm leaning towards option 1 at the moment. However, admittedly, I have not thought very deeply about what other container-related extensions may come in the future. If there are specific ideas or examples related to containers you could share, that would be helpful.

@iskiselev
Copy link
Contributor

@alanwest, I like ide about small targeted packages and meta-package on top if needed. I like it especially for containers, as it is hard to imagine anything beyond resource detector now - as docker should not affect on anything executed inside, and if there will be some tool that specially manages docker/containers (and will require additional instrumentation for docker API calls), it probably will be highly-specialized that better to instrument with special package.
So, my vote will be for option 1.

iskiselev added a commit to iskiselev/opentelemetry-dotnet-contrib that referenced this issue 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)
@Kielek Kielek changed the title Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetector.Container Rename OpenTelemetry.Extensions.Docker to OpenTelemetry.ResourceDetectors.Container Mar 14, 2023
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
Projects
None yet
4 participants