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

sdk/trace: removing ApplyConfig and Config #1693

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

ijsong
Copy link
Contributor

@ijsong ijsong commented Mar 11, 2021

This patch removes ApplyConfig method and Config struct from go.opentelemetry.io/otel/sdk/trace package. To ensure valid config for TracerProvider, it adds ensureValidTracerProviderConfig private function.

Jaeger and Zipkin have been used the Config directly across package boundaries. Since Config is removed, they can't use it. This change, thus, replaces WithSDK with WithSDKOptions.

Resolves #1636, #1705.

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #1693 (57fd05a) into main (1d42be1) will increase coverage by 0.0%.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #1693   +/-   ##
=====================================
  Coverage   77.8%   77.9%           
=====================================
  Files        130     131    +1     
  Lines       7013    6997   -16     
=====================================
- Hits        5463    5451   -12     
+ Misses      1299    1296    -3     
+ Partials     251     250    -1     
Impacted Files Coverage Δ
exporters/trace/jaeger/jaeger.go 93.5% <100.0%> (-0.3%) ⬇️
exporters/trace/zipkin/zipkin.go 72.9% <100.0%> (-0.4%) ⬇️
sdk/trace/config.go 100.0% <100.0%> (ø)
sdk/trace/provider.go 83.9% <100.0%> (+0.8%) ⬆️
sdk/trace/span.go 90.9% <100.0%> (ø)

@ijsong ijsong force-pushed the remove-config branch 3 times, most recently from 433342a to e38c0fd Compare March 11, 2021 16:18
@ijsong ijsong marked this pull request as ready for review March 11, 2021 16:45
Copy link
Member

@Aneurysm9 Aneurysm9 left a comment

Choose a reason for hiding this comment

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

I think I'd like to see a different solution to getting TracerProviderOptions from the exporter pipeline convenience methods to the TracerProvider initialization before landing this.

exporters/trace/jaeger/jaeger.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/model.go Outdated Show resolved Hide resolved
@Aneurysm9
Copy link
Member

@ijsong sorry about the churn on this, there were several other issues working in the same area though I think we've made it through most of them.

@open-telemetry/go-approvers can we get another approval on this so we can land it?

@ijsong
Copy link
Contributor Author

ijsong commented Mar 16, 2021

@Aneurysm9 No problem, I fixed some issues after merging main branch, and renamed field name defaultSampler to sampler to prepare #1702.

exporters/trace/jaeger/jaeger_test.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/zipkin.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/zipkin_test.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/zipkin_test.go Outdated Show resolved Hide resolved
exporters/trace/zipkin/zipkin_test.go Outdated Show resolved Hide resolved
sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/provider.go Outdated Show resolved Hide resolved
sdk/trace/span.go Show resolved Hide resolved
This patch removes `ApplyConfig` method and `Config` struct from
`go.opentelemetry.io/otel/sdk/trace` package.  To ensure valid config
for TracerProvider, it adds `ensureValidTracerProviderConfig` private
function.
Jaeger and Zipkin have been used the `Config` directly across package
boundaries. Since `Config` is removed, they can't use it. This change,
thus, replaces `WithSDK` with `WithSDKOptions`.

Resolves open-telemetry#1636, open-telemetry#1705.
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.

Remove TracerProvider.ApplyConfig and Config
5 participants