opentelemetry-java icon indicating copy to clipboard operation
opentelemetry-java copied to clipboard

Break out :exporters:common module

Open jack-berg opened this issue 2 years ago • 9 comments

Related to comment in #4501.

Unfortunately, we still need to keep :exporters:otlp:common since the serialization logic is used in :expoters:logging-otlp.

jack-berg avatar Jun 30 '22 17:06 jack-berg

Codecov Report

Merging #4575 (ebe741e) into main (91bd17e) will increase coverage by 0.01%. The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #4575      +/-   ##
============================================
+ Coverage     90.08%   90.10%   +0.01%     
  Complexity     5074     5074              
============================================
  Files           584      584              
  Lines         15667    15667              
  Branches       1504     1504              
============================================
+ Hits          14114    14117       +3     
+ Misses         1100     1099       -1     
+ Partials        453      451       -2     
Impacted Files Coverage Δ
...lemetry/exporter/internal/ExporterBuilderUtil.java 100.00% <ø> (ø)
...entelemetry/exporter/internal/ExporterMetrics.java 100.00% <ø> (ø)
...va/io/opentelemetry/exporter/internal/TlsUtil.java 86.27% <ø> (ø)
...elemetry/exporter/internal/auth/Authenticator.java 100.00% <ø> (ø)
...telemetry/exporter/internal/grpc/GrpcExporter.java 100.00% <ø> (ø)
...ry/exporter/internal/grpc/GrpcExporterBuilder.java 93.82% <ø> (ø)
...metry/exporter/internal/grpc/GrpcExporterUtil.java 80.00% <ø> (ø)
...emetry/exporter/internal/grpc/GrpcRequestBody.java 100.00% <ø> (ø)
...lemetry/exporter/internal/grpc/GrpcStatusUtil.java 100.00% <ø> (ø)
...try/exporter/internal/grpc/ManagedChannelUtil.java 86.20% <ø> (ø)
... and 26 more

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

codecov[bot] avatar Jun 30 '22 17:06 codecov[bot]

This new module ends up being 100% internal classes, correct? Can we maybe merge it into the dependent jars at build time so we don't have to publish it?

jkwatson avatar Jul 05 '22 21:07 jkwatson

Can we maybe merge it into the dependent jars at build time so we don't have to publish it?

Merging would probably make sense if there is only one dependent jar, though in that case we would have inlined the code in. For multiple dependent jars, it seems tricky (rename the package when inlining for each? etc). I wouldn't make it too complex.

anuraaga avatar Jul 06 '22 04:07 anuraaga

Can we maybe merge it into the dependent jars at build time so we don't have to publish it?

Merging would probably make sense if there is only one dependent jar, though in that case we would have inlined the code in. For multiple dependent jars, it seems tricky (rename the package when inlining for each? etc). I wouldn't make it too complex.

oh yeah. we'd have to repackage to match the package of what we were merging to each time. ugh.

jkwatson avatar Jul 06 '22 23:07 jkwatson

I can't tell on github, but does this end up with overlapping package names with the module you moved stuff from? We need to make sure they both export the same package, or the 2 jars will collide if using the Java Module System.

jkwatson avatar Jul 06 '22 23:07 jkwatson

I can't tell on github, but does this end up with overlapping package names with the module you moved stuff from? We need to make sure they both export the same package, or the 2 jars will collide if using the Java Module System.

The package names of all the classes that where moved from :exporters:otlp:common to :exporters:common are the same as before: io.opentelemetry.exporter.internal.*.

The remaining classes in :exporters:otlp:common are all in io.opentelemetry.exporter.internal.otlp.*.

jack-berg avatar Jul 18 '22 23:07 jack-berg

I can't tell on github, but does this end up with overlapping package names with the module you moved stuff from? We need to make sure they both export the same package, or the 2 jars will collide if using the Java Module System.

The package names of all the classes that where moved from :exporters:otlp:common to :exporters:common are the same as before: io.opentelemetry.exporter.internal.*.

The remaining classes in :exporters:otlp:common are all in io.opentelemetry.exporter.internal.otlp.*.

Cool. Just wanted to make sure that our JMS users wouldn't get broken packages.

jkwatson avatar Jul 18 '22 23:07 jkwatson

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

github-actions[bot] avatar Aug 04 '22 18:08 github-actions[bot]

Looks like this needs some conflicts resolved, @jack-berg

jkwatson avatar Aug 05 '22 23:08 jkwatson