[exporter/otlp] split into its own module

[codeboten]

Fixes #6195

[codecov]

Codecov Report

Base: 92.04% // Head: 91.94% // Decreases project coverage by -0.09% ⚠️

Coverage data is based on head (585d5f6) compared to base (6313054).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6228      +/-   ##
==========================================
- Coverage   92.04%   91.94%   -0.10%     
==========================================
  Files         219      219              
  Lines       13245    13245              
==========================================
- Hits        12191    12178      -13     
- Misses        830      843      +13     
  Partials      224      224              

Impacted Files	Coverage Δ
exporters/otlpgrpcexporter/config.go	0.00% <ø> (ø)
exporters/otlpgrpcexporter/factory.go	90.90% <ø> (ø)
exporters/otlpgrpcexporter/otlp.go	90.42% <ø> (ø)
cmd/otelcorecol/components.go	63.63% <100.00%> (ø)
config/configgrpc/configgrpc.go	87.98% <0.00%> (-4.81%)	⬇️
exporter/exporterhelper/traces.go	85.00% <0.00%> (-1.67%)	⬇️
exporter/exporterhelper/queued_retry.go	95.23% <0.00%> (-0.74%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bogdandrutu]

I think we should send also a PR to remove https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/doc.go since it belongs (will) to a module without anything in that parent directory. Also think what we want to do with https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#general-information since it feels strange that it belongs to a different module.

Maybe we create a module "exporter" with all the exporters?

[codeboten]

I think we should send also a PR to remove https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/doc.go since it belongs (will) to a module without anything in that parent directory

I agree this makes sense once all the other exporters are split into their own modules.

Also think what we want to do with https://github.com/open-telemetry/opentelemetry-collector/tree/main/exporter#general-information since it feels strange that it belongs to a different module.

This could likely be moved under the docs directory in something called exporter.md? Maybe there should be a components directory under docs?

Maybe we create a module "exporter" with all the exporters?

What would the result of someone including that module be? All the exporters get pulled in? I wonder if it would be confusing for users as it may be unclear what exporters fall under that package.

[bogdandrutu]

What would the result of someone including that module be? All the exporters get pulled in? I wonder if it would be confusing for users as it may be unclear what exporters fall under that package.

"All the exporters get pulled in" pulled in where? All the exporters are available in the module, if you talk about the builder you need 2 lines one for module and one for import.

[bogdandrutu]

last one about this "structure" part:
I saw somewhere in contrib that go mod complains if you have a module go.opentelemetry.io/collector/exporter (in case in the future we want to use that package and module for some shared code between exporters) and go.opentelemetry.io/collector/exporter/otlpexporter. Can you check that?

The error:

Running target 'tidy' in module 'internal/aws/xray' as part of group 'internal'
/Library/Developer/CommandLineTools/usr/bin/make -C internal/aws/xray tidy
rm -fr go.sum
go mod tidy -compat=1.18
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

Is this something that we want to support in the future? Where do we consider to put a package for "common" things between exporters? One of the thought I had was that component.Exporter and component.ExporterFactory can/may be moved there?

[bogdandrutu]

It looks to be related to this issue golang/go#30590, found starting from the code :)) https://cs.opensource.google/go/go/+/master:src/cmd/go/testdata/script/mod_dot.txt;l=30;bpv=1;bpt=0

[codeboten]

Another update to this PR here:

• moved the exporter to exporters directory. this is more inline with opentelemetry-go: https://github.com/open-telemetry/opentelemetry-go/tree/main/exporters
• renamed otlpexporter to otlpgrpcexporter to make this more consistent with otlphttpexporter

Please take a look @open-telemetry/collector-approvers

[dmitryax]

Renaming modules/packages is a significant breaking change for the API consumers. I believe this should be done with aliasing first. We should keep the old one for now. And create new module only with the references to the old package (not otherwise to prevent circular deps)

[dmitryax]

Also I'm still not convinced that we have significant enough reasons to rename exporter to exporters. Even if I like the plural more, I think we should clearly provide the reasoning behind that in this PR or an issue. Probably better to hav e a separate PR for that. This one is just about otlp exporter. Looks like the errors that @bogdandrutu posted are not relevant anymore?

[dmitryax]

Do we rename the package only but keep otlp as the component name that used in configs?

[codeboten]

otlp in the config applies to both grpc and http exporters, my opinion is that it's fine to separate config from package name, but i don't feel strongly about it. i can revert if that's preferable.

[dmitryax]

otlp in the config applies to both grpc and http exporters,

otlphttpexporter uses otlphttp https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/otlphttpexporter/factory.go#L33

I think we should keep module names consistent with the collector component names. I would vote for keeping otlpexporter without renaming it. This seems like the most important one

[codeboten]

sorry i was thinking of the receiver 😅

[mx-psi]

I think we should keep it as otlpexporter. It would otherwise be the only exporter with a mismatch between the Go module name and the configuration identifier.

[codeboten]

Also I'm still not convinced that we have significant enough reasons to rename exporter to exporters. Even if I like the plural more, I think we should clearly provide the reasoning behind that in this PR or an issue. Probably better to have a separate PR for that. This one is just about otlp exporter. Looks like the errors that @bogdandrutu posted are not relevant anymore?

Since this is the first time we'll publish these components as separate modules, it's the time to decide on what makes the most sense. The plural does make more sense if it's going to contain more than one exporter, and as i mentioned it does align with otel-go.

That being said, I agree that deprecating/breaking the import path for users shouldn't be done lightly.

[codeboten]

@dmitryax I opened an issue to discuss moving components to the plural subdirectory: #6337

[bogdandrutu]

Looks like the errors that @bogdandrutu posted are not relevant anymore?

@dmitryax the errors I mentioned will be removed if we rename the package since, "exporter" package will be free to use.

Also I'm still not convinced that we have significant enough reasons to rename exporter to exporters.

@dmitryax can you confirm that the errors I mentioned are ok long term?

[codeboten]

Closing in favour of #6343