Move components under new directory structure

[codeboten]

The discussion around this was started at the SIG meeting on Oct 12, and can be partially found in this PR. Part of the work to publish components as separate go modules gives us an opportunity to decide on a path that makes most sense for these components.

Currently packages are imported as components of the collector package. During the discussion it was proposed that components be moved to a plural directory. For example, the OTLP exporter would be moved from:
go.opentelemetry.io/collector/exporter/otlpexporter
to
go.opentelemetry.io/collector/exporters/otlpexporter

This provides a hint to users that there are additional exporters, and follows the example set by otel-go

The proposal is that all components (receivers, processors, exporters, extensions) follow this pattern. For consistency, the same pattern would be applied to the components in the contrib repository.

[Aneurysm9]

I wouldn't take the otel-go example as a north star or even as very meaningful. It was changed following a mistakenly-published v1.0.0 tag before retract statements could be added to go.mod files.

Moving modules around in otel-go has also caused a number of problems as systems that depend on no-longer-extant modules and packages lead to confusing error messages. We still don't have a resolution to this issue.

I would suggest that we change as little as possible, even if the grammar is occasionally annoying.

[bogdandrutu]

This provides a hint to users that there are additional exporters, and follows the example set by otel-go

That is not the only reason, I believe the suggestion started from this issue golang/go#30590.

[mx-psi]

I went through other language libraries to see what they did. There seems to be no alignment on this front. The situation is as follows:

1. Using exporter:
   • Python
   • .NET (namespace is OpenTelemetry.Exporter)
   • Ruby
2. Using exporters:
   • Java
   • Go
   • Swift
3. Flat structure:
   • Rust
   • Erlang (everything is under apps)
   • Javascript (everything under packages)
4. Other:
   • PHP (exporters under contrib, other stuff on other folders)

Another question (maybe you discussed on the SIG meeting) is what to do with contrib. I think internal consistency between contrib and core is more important than consistency with OTel Go.

[codeboten]

@mx-psi i agree consistency across core/contrib is important. The suggestion to move things to a plural directory would have an impact on contrib as well, the same work would need to be done there.

I opened a PR #6343 to test using the exporter module separately once the other exporters are split out. @bogdandrutu i don't see the linting error mentioned in the golang issue. PTAL

[bogdandrutu]

@bogdandrutu i don't see the linting error mentioned in the golang issue. PTAL

More people saw that in the component I mentioned. So not sure what is your setup, I saw also that if you don't have latest golang compiler (>1.19) you don't see it.

[codeboten]

So digging into this a bit further, I'm convinced that the errors are showing up because of the makefile using find to get a list of directories to inspect via go list. I tested this as far as go 1.13 and it was showing an error even back then

/app/internal/aws/xray# make SHELL='sh -x' tidy
+ + go list -f {{ .Dir }} ./...
sort
+ find /app/internal/aws/xray -name *.go -not -path */third_party/* -not -path */local/* -type f
+ sort
+ find /app/internal/aws/xray -name *.md -o -name *.go -o -name *.yaml -not -path */third_party/*+  -type f
sort
+ go list /app/internal/aws/xray/ /app/internal/aws/xray/testdata/ /app/internal/aws/xray/testdata/sampleapp/ /app/internal/aws/xray/testdata/sampleserver/
main module (github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/xray) does not contain package github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/xray/testdata/sampleapp
main module (github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/xray) does not contain package github.com/open-telemetry/opentelemetry-collector-contrib/internal/aws/xray/testdata/sampleserver
rm -fr go.sum
+ rm -fr go.sum
go mod tidy -compat=1.18
+ go mod tidy -compat=1.18

This is the error under go 1.13:

make tidy
go: directory testdata/sampleapp is outside main module
go: directory testdata/sampleserver is outside main module

I was able to reproduce the same warnings by copying over a snippet of the Makefile over from contrib and got the following warnings in this PR:

main module (go.opentelemetry.io/collector/exporter) does not contain package go.opentelemetry.io/collector/exporter/loggingexporter
main module (go.opentelemetry.io/collector/exporter) does not contain package go.opentelemetry.io/collector/exporter/loggingexporter/internal/otlptext
main module (go.opentelemetry.io/collector/exporter) does not contain package go.opentelemetry.io/collector/exporter/otlpexporter
main module (go.opentelemetry.io/collector/exporter) does not contain package go.opentelemetry.io/collector/exporter/otlphttpexporter

If you look at the message from the warning, it makes sense that go list throws a warning as we're asking it to inspect submodules which are not part of the current module because find returns a list of all the subdirectories regardless of whether they have their own go.mod.

[bogdandrutu]

So you think it is not a problem with that pattern. That is good to know.

[codeboten]

So you think it is not a problem with that pattern. That is good to know.

That's my interpretation. I do think this uncovers a bug in Makefile.common here: https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/95b8b35989af11416194bbdd9c0a81fa6d8793a3/Makefile.Common#L36

If you look at the original commit, it was meant to find only the files for the current module:

ALL_SRC changed to find .go files of the current module only.

But with submodules inside the same directory, it's getting more than what it's intended.

[jpkrohling]

I have mixed feelings about the usage of plural vs. singular. Looking at the final package, I'd prefer it to use the singular mode (exporter/otlphttpexporter), but when browsing the code on GitHub, I would prefer to see "exporters/" instead.

In trying to find more authoritative opinions on this, I found this blog post from @rakyll: https://rakyll.org/style-packages/ . It could expand a bit on the reasoning, and it's somewhat old, but I don't think it's outdated. I also found a couple of other opinions pointing in the same direction, but I'm not sure how reputable those sources are, so I'll leave them out.

If I were to pick one, I would stay with the singular mode: it's the least friction right now and what seems to be common practice in the Go world.

[bogdandrutu]

The link you put is mostly about the final part of the package, the one that is usually used in code like otlphttpexporter.NewFactory. But I am ok with keeping it, as long as I can have a module go.opentelemetry.io/collector/exporter that is independent from go.opentelemetry.io/collector/exporter/otlphttpexporter and is golang accepted (no errors).

[bogdandrutu]

@jpkrohling funny enough that golang official packages have couple of plurals like "strings" and "bytes" :)))

[jpkrohling]

Yeah, that was mentioned in a few of the blog posts I read as well, with the reasoning that the singular versions are reserved keywords.

[codeboten]

Closing this issue as components have already been moved. See #6343