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

Refactor the Duplicated code in OTLP Exporter with config part #2684

Conversation

hanyuancheung
Copy link
Member

@hanyuancheung hanyuancheung commented Mar 19, 2022

Fix: part of #2641

  • Refactor otlpconfig part from exporter/otlp/otlpmetric(otlptrace)/internal/ into exporter/otlp/internal/envconfig
  • Refactor transform part from exporter/otlp/otlpmetric(otlptrace)/internal/ into exporter/otlp/internal/transform
  • Refactor connection part from exporter/otlp/otlpmetric(otlptrace)/internal/ exporter/otlp/internal/connection

@codecov
Copy link

codecov bot commented Mar 19, 2022

Codecov Report

Merging #2684 (a87614c) into main (4399e22) will increase coverage by 0.0%.
The diff coverage is 92.8%.

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #2684    +/-   ##
======================================
  Coverage   76.7%   76.7%            
======================================
  Files        181     177     -4     
  Lines      12175   12066   -109     
======================================
- Hits        9345    9263    -82     
+ Misses      2605    2579    -26     
+ Partials     225     224     -1     
Impacted Files Coverage Δ
exporters/otlp/internal/envconfig/config.go 100.0% <ø> (ø)
exporters/otlp/internal/envconfig/tls.go 62.5% <ø> (ø)
...xporters/otlp/otlpmetric/otlpmetricgrpc/options.go 76.4% <78.9%> (ø)
exporters/otlp/otlptrace/otlptracegrpc/options.go 76.4% <78.9%> (ø)
exporters/otlp/internal/envconfig/options.go 81.7% <86.0%> (ø)
...xporters/otlp/otlpmetric/otlpmetrichttp/options.go 72.9% <92.8%> (ø)
exporters/otlp/internal/envconfig/envconfig.go 98.3% <100.0%> (+2.7%) ⬆️
exporters/otlp/otlpmetric/otlpmetricgrpc/client.go 97.7% <100.0%> (ø)
exporters/otlp/otlpmetric/otlpmetrichttp/client.go 76.7% <100.0%> (ø)
exporters/otlp/otlptrace/otlptracegrpc/client.go 92.1% <100.0%> (ø)
... and 3 more

@hanyuancheung hanyuancheung changed the title Refactor the Duplicated code in OTLP Exporter Refactor the Duplicated code in OTLP Exporter with config part Mar 19, 2022
Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to extract this out to a common base between metrics and traces we need to be extra careful that dependencies don't leak between them, or out into the API.

This won't be acceptable until the exporter's dependencies are contained within the exporter only.

go.mod Outdated
Comment on lines 10 to 12
go.opentelemetry.io/otel/exporters/otlp/internal/retry v0.0.0-00010101000000-000000000000
go.opentelemetry.io/otel/trace v1.5.0
google.golang.org/grpc v1.45.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change will add the otlp internals to every project that depends on the otel API. Which is why every example and bridge just gained 40-50 lines of dependences in the go.sum files.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I didn't notice when this dependency was introduced. Do you have some suggestions to resolve this problem? @MadVikingGod

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My guess is that exporters/otlp/internal is under the main otlp mod (/go.mod) so when you moved these there you brought with it the grpc etc dependencies.

To house it there you will need a mod somewhere, either /exporter/otlp, /exporters/otlp/internal or /exporters/otlp/internal/pkg. I would experiment what works best and also remember to add it to the versions.yaml and run make crosslink or make precommit.

@hanyuancheung
Copy link
Member Author

Tnanks for advice, I've added the go.mod in exporters/otlp/internal, it seems to look good. cc @MadVikingGod

Copy link
Contributor

@MadVikingGod MadVikingGod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other then the Changelog LGTM

Comment on lines +28 to +29
- The metrics API has been significantly changed. (#2587)
- Refactored the duplicated part in OTLP exporter trace and metrics config. (#2684)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should go in the unreleased section, and only have this PR's change.

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.

2 participants