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

Decouple otlp/otlptrace/internal from otlp/internal using gotmpl #4397

Merged
merged 13 commits into from
Aug 2, 2023

Conversation

MrAlias
Copy link
Contributor

@MrAlias MrAlias commented Aug 1, 2023

Currently the otlp exporters share internal packages across package boundaries. This is something we want to stop doing: #3846. This PR addresses the cross-module internal package use for the otel/exporters/otlp/otlptrace package (but not its sub-modules).

  • Add the gotmpl tooling to the project
  • Add an otel/internal/shared package to hold all templates rendered by the gotmpl tooling
    • Add the otel/exporters/otlp/internal/retry package to the shared templates
    • Add the otel/exporters/otlp/internal/envconfig package to the shared templates
    • Add the otel/exporters/otlp/otlptrace/internal package to the shared templates
  • Use gotmpl to render the new retry and envconfig packages in otel/exporters/otlp/otlptrace/internal
  • Use gotmpl to render the rest of otel/exporters/otlp/otlptrace/internal so it can use the now local retry and envconfig packages.
  • Update otlptracegrpc and otlptracehttp to use the otel/exporters/otlp/otlptrace/internal packages

What is not included

Both the otlptracegrpc and otlptracehttp packages can now also have copies of these (and other) packages. This will decouple both packages from the otlptrace/internal packages.

The plan once this is merged is to tackle each of those packages in their own PRs.

Why not skip otlptrace/internal and just do everything in otlptracegrpc/internal and otlptracehttp/internal?

The otlptrace/internal package is planned to be flattened into both. However, for backwards compatibility reasons otlptrace/internal cannot be removed even after the sub-modules no longer depend on it.

Given otlptrace/internal will need to exist in the future until a major version bump, it is updated here to at least not have a cross module internal package dependency (other than otel/internal/global).

@MrAlias MrAlias added the pkg:exporter:otlp Related to the OTLP exporter package label Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #4397 (40215d1) into main (c4d2d7c) will increase coverage by 0.0%.
The diff coverage is 91.2%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #4397    +/-   ##
======================================
  Coverage   83.5%   83.6%            
======================================
  Files        184     186     +2     
  Lines      14448   14629   +181     
======================================
+ Hits       12073   12238   +165     
- Misses      2145    2157    +12     
- Partials     230     234     +4     
Files Changed Coverage Δ
exporters/otlp/otlptrace/internal/header.go 100.0% <ø> (ø)
...rs/otlp/otlptrace/internal/otlpconfig/envconfig.go 89.5% <ø> (ø)
...xporters/otlp/otlptrace/internal/otlpconfig/tls.go 62.5% <ø> (ø)
...tlp/otlptrace/internal/tracetransform/attribute.go 100.0% <ø> (ø)
...lptrace/internal/tracetransform/instrumentation.go 100.0% <ø> (ø)
...otlp/otlptrace/internal/tracetransform/resource.go 100.0% <ø> (ø)
...ers/otlp/otlptrace/internal/tracetransform/span.go 96.5% <ø> (ø)
exporters/otlp/otlptrace/otlptracegrpc/client.go 92.5% <ø> (ø)
exporters/otlp/otlptrace/otlptracegrpc/options.go 76.4% <ø> (ø)
exporters/otlp/otlptrace/otlptracehttp/client.go 77.5% <ø> (ø)
... and 4 more

... and 2 files with indirect coverage changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:exporter:otlp Related to the OTLP exporter package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants