-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Remove TracerProvider.ApplyConfig and Config #1636
Comments
If
opentelemetry-go/sdk/trace/provider.go Lines 173 to 185 in 9d3416c
By the way, I can't find any rules of range of the |
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
See this discussion from the "otel-go" channel in the "Cloud Native Computing Foundation" Slack workspace for a motivation for using the In particular, I'd like to be able to use tracing while discovering and collecting more fields for the |
@seh, Could you use
|
@seh would a |
@ijsong while resources need to be immutable types, the one associated with the TracerProvider is not required to be static. We are removing the |
As noted in the parallel Slack discussion, a |
Although I could misunderstand the specification, my understanding of it is like these:
For 1,
For 2,
For 3 and 4,
For 5,
For 6,
According to these, creating a new |
@MrAlias @seh |
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
Yikes, 😬, that is not a normative requirement in the specification yet it seems it is commonly being interpreted as such. That's not great. Based on the ambiguity, I agree we should take the conservative approach here and try and honor the possible interpretation that the author was trying to make a normative requirement here. |
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, removes `WithSDK` functions from them, instead, introduces new functions, for instance, `WithResource`, `WithDefaultSampler`, and `WithSpanLimits`. Resolves open-telemetry#1636.
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, replcae `WithSDK` with `WithSDKOptions`. Resolves open-telemetry#1636.
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, replcae `WithSDK` with `WithSDKOptions`. Resolves open-telemetry#1636, open-telemetry#1705.
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.
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.
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.
The
TracerProvider.ApplyConfig
method and relatedConfig
are additions to the exposed API that likely are not needed nor ideal. TheApplyConfig
method exists to update aTracerProvider
's configuration. This is an optional part of the specification:meaning that we could remove these items and remain compliant with the specification.
Removing this type and method from the exported API will be a reduction in the API. This will reduce the on-boarding load for new users wondering when they should be configuring the
TracerProvider
(at creation or after with this method). Additionally, it could reduce the maintenance burden if we decide in the future to add direct Setters for theConfig
fields to avoid issues similar to the ones identified in #1631 but would still need to support this exported method and type.Removing these will remove functionality from the
TracerProvider
API, the user will no longer be able to update a configuration of a TracerProvider after it has been created. There do not seem to be any reasons why we would need to update this configuration after creation so this seems justified. If this functionality needs to be added back in we could likely do so in a way that better serves the intended purpose and one that doesn't leak internal implementation details of how theTracerProvider
manages a synchronized configuration.Proposal
Removing the
ApplyConfig
method from theTracerProvider
will make the configuration of span limits, the default sampler, ID generators, and resource immutable. This means that theconfig
field of theTracerProvider
which holds an atomic value of the relatedConfig
could be flattened into the fields of aConfig
directly.Removing this field and with the merging of #1633 the
Config
type will be unused. It can be removed when this is done.The text was updated successfully, but these errors were encountered: