Skip to content

Break out :exporters:common module#4575

Merged
jack-berg merged 7 commits intoopen-telemetry:mainfrom
jack-berg:exporter-common
Aug 8, 2022
Merged

Break out :exporters:common module#4575
jack-berg merged 7 commits intoopen-telemetry:mainfrom
jack-berg:exporter-common

Conversation

@jack-berg
Copy link
Copy Markdown
Member

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 jack-berg requested a review from a user June 30, 2022 17:26
@jack-berg jack-berg requested a review from Oberon00 as a code owner June 30, 2022 17:26
@codecov
Copy link
Copy Markdown

codecov bot commented Jun 30, 2022

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.

@jkwatson
Copy link
Copy Markdown
Contributor

jkwatson commented Jul 5, 2022

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?

@anuraaga
Copy link
Copy Markdown
Contributor

anuraaga commented Jul 6, 2022

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.

@jkwatson
Copy link
Copy Markdown
Contributor

jkwatson commented Jul 6, 2022

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
Copy link
Copy Markdown
Contributor

jkwatson commented Jul 6, 2022

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.

@jack-berg
Copy link
Copy Markdown
Member Author

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.*.

@jkwatson
Copy link
Copy Markdown
Contributor

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 4, 2022

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

@github-actions github-actions bot added the Stale label Aug 4, 2022
@jack-berg jack-berg removed the Stale label Aug 4, 2022
@jkwatson
Copy link
Copy Markdown
Contributor

jkwatson commented Aug 5, 2022

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

@jack-berg jack-berg merged commit 77be2e0 into open-telemetry:main Aug 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants