Log SDK, OTLP builders to accept Resource directly instead of wrapping in Config

[cijothomas]

Logs sdk and otlp pipeline builders now accept Resource directly in their builders, instead of having to wrap inside Config. For logs, Resource was the only thing inside Config anyway, but even if there were more concepts to be added later, the builders can accept them directly. (If, more memory efficiency reasons, we need to wrap things under, it can be an internal implementation detail, achievable without public api changes.)

Tracing still has Config with idgen,sampler, resource etc, and builders accepting Config only. It is a bit odd to use, as Config itself has own builders but without the build() method! If there is agreement, I can do similar changes for Traces too.

[codecov]

Codecov Report

Attention: Patch coverage is 66.66667% with 16 lines in your changes are missing coverage. Please review.

Project coverage is 73.6%. Comparing base (d21b13a) to head (066e176).

Files	Patch %	Lines
opentelemetry-otlp/src/logs.rs	0.0%	15 Missing ⚠️
opentelemetry-sdk/src/logs/log_emitter.rs	96.4%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #1788     +/-   ##
=======================================
- Coverage   73.6%   73.6%   -0.1%     
=======================================
  Files        124     123      -1     
  Lines      19517   19500     -17     
=======================================
- Hits       14378   14362     -16     
+ Misses      5139    5138      -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lalitb]

If there is agreement, I can do similar changes for Traces too.

This makes sense to me, I don't think we need Config, when there is already a builder for LoggerProvider/TracerProvider.

[TommyCpp]

It is a bit odd to use, as Config itself has own builders but without the build() method

It's actually not meant to be a Builder. Rather than setter functions for Config. Do you see an issue with this pattern? I don't see one way is better than the other but given we already implement the Config for traces I don't think we need to change it

[cijothomas]

It is a bit odd to use, as Config itself has own builders but without the build() method

It's actually not meant to be a Builder. Rather than setter functions for Config. Do you see an issue with this pattern? I don't see one way is better than the other but given we already implement the Config for traces I don't think we need to change it

The config feels like an unnecessary additional layering without adding any value.

specifying Resource, via config:

     .with_config(
            Config::default().with_resource(Resource::new(vec![KeyValue::new(
                "service.name",
                "log-appender-tracing-example",
            )])),
        )

vs directly specifying Resource. This looks cleaner in my opinion.

.with_resource(Resource::new(vec![KeyValue::new(
            "service.name",
            "log-appender-tracing-example",
        )]))

[TommyCpp]

It is a bit odd to use, as Config itself has own builders but without the build() method

It's actually not meant to be a Builder. Rather than setter functions for Config. Do you see an issue with this pattern? I don't see one way is better than the other but given we already implement the Config for traces I don't think we need to change it

The config feels like an unnecessary additional layering without adding any value.

In practice the only difference is the option 1 requires users to be explicit on Config::default. I don't see that as a huge issue performance wise or readbility wise. At least not warrant the cost of swiching traces implementation over. And we should keep configuration pipeline consistent so not sure we want to have Config in traces but not in logs.

We can discuss more on the community call if needed

[cijothomas]

Do you see an issue with this pattern?

Yes, but to be honest, lack of consistency is my main worry! Examples

1. Traces use it, but not Metrics. (Logs used it, but this PR is removing it for logs)
2. Within Traces, there is trace_config and batch_config. The latter is configurable via builder pattern, but the former is not.

Other than that, it is not clear what value it offers to end users.
For example,

provider.with_sampler(sampler).with_resource(resource)

vs

.with_trace_config(sdktrace::config().with_resource(resource).with_sampler(sampler))

Not every config for provider is done via trace_config - eg: Processors are done differently, so users must now use both patterns to configure the provider. It feels a lot cleaner to offer every config via the existing builder pattern for provider_builder (and do it consistently across all signals/exporters. I'll propose additional changes to the way OTLP Pipeline is configured now in a separate PR.)

[TommyCpp]

Traces use it, but not Metrics. (Logs used it, but this PR is removing it for logs)

It we want to converage into a single implementation shouldn't we do breaking changes on the signals that is less stable, in this case, metrics?

Within Traces, there is trace_config and batch_config. The latter is configurable via builder pattern, but the former is not.

Batch config was consistent with the trace config until #1480 introduces a builder pattern. Given there are little logic from that builder, we should probably revert the changes.

so users must now use both patterns to configure the provider.

I am not sure what's the two patterns here? I see Config as a simple data holder with mutation functions. While TracerProvider uses builder pattern this doesn't mean every struct requires a builder pattern. For example, TracerProviderBuilder probably shouldn't include logic of chosing exporters for processors

[cijothomas]

Traces use it, but not Metrics. (Logs used it, but this PR is removing it for logs)

It we want to converage into a single implementation shouldn't we do breaking changes on the signals that is less stable, in this case, metrics?

Within Traces, there is trace_config and batch_config. The latter is configurable via builder pattern, but the former is not.

Batch config was consistent with the trace config until #1480 introduces a builder pattern. Given there are little logic from that builder, we should probably revert the changes.

so users must now use both patterns to configure the provider.

I am not sure what's the two patterns here? I see Config as a simple data holder with mutation functions. While TracerProvider uses builder pattern this doesn't mean every struct requires a builder pattern. For example, TracerProviderBuilder probably shouldn't include logic of chosing exporters for processors

The 2 patterns are:

1. For adding processors, provider.with_processor or add_processor. (users don't go through config)
2. For adding resource, provider.with_config, with Resource embedded inside config. (users have to go through config)

For example, TracerProviderBuilder probably shouldn't include logic of chosing exporters for processors

I think you probably meant to say "logic of chosing processors (simple vs batch) for exporters" ? I mostly agree. Exporter is not something Provider should be aware of - provider only knows of processor/readers, and they in turn deal with exporter. I have some ideas to fix that.

Even with that - there is still with_processor and other things like with_config(sampler,id etc.). What value does with_config provides compared to using the builder pattern on top of provider?

[cijothomas]

@lalitb @TommyCpp please review (re-review)

[cijothomas]

As discussed offline with Zhongyang:

1. We need to re-evaluate the whole OTLP Pipeline idea. It is not composable (eg: not easy for users to add an extra EnrichingLogProcessor or RedactorProcessor before the pipeline!) and is very different from stdout exporter (and very diff. from other OTel languages)
2. This PR is good to do as-is. Cijo will send a proposal to refactor OTLP Pipeline for logs. This makes Logs and Traces drift apart, but we can afford to leverage Logs to liberally experiment things, given Logs is still new and alpha. If things makes sense, we should absorb it for Traces too, but make sure we Obsolete/Deprecate first, then remove to ease user pain, given Tracing has been around for a while.
3. Zhongyang is also exploring how best to re-use the data+transport for all signals - for eg: we should be able to use a single connection for logs/traces/metrics instead of opening 3 connections, each with diff policies for retry/auth etc. This "cross-cutting" part is relatively hard problem, but something worth solving.

@TommyCpp Please feel to correct the above, if I captured everything correct or not!

[lalitb]

LGTM, and agree with further refactoring of OTLP*Pipeline as discussed here.

[TommyCpp]

nit: can link the Resource doc from sdk

[cijothomas]

As discussed offline with Zhongyang:

1. We need to re-evaluate the whole OTLP Pipeline idea. It is not composable (eg: not easy for users to add an extra EnrichingLogProcessor or RedactorProcessor before the pipeline!) and is very different from stdout exporter (and very diff. from other OTel languages)
2. This PR is good to do as-is. Cijo will send a proposal to refactor OTLP Pipeline for logs. This makes Logs and Traces drift apart, but we can afford to leverage Logs to liberally experiment things, given Logs is still new and alpha. If things makes sense, we should absorb it for Traces too, but make sure we Obsolete/Deprecate first, then remove to ease user pain, given Tracing has been around for a while.
3. Zhongyang is also exploring how best to re-use the data+transport for all signals - for eg: we should be able to use a single connection for logs/traces/metrics instead of opening 3 connections, each with diff policies for retry/auth etc. This "cross-cutting" part is relatively hard problem, but something worth solving.

@TommyCpp Please feel to correct the above, if I captured everything correct or not!

#1810 Opened this issue to get some feedback.