Change resource.New() to use functional options; add builtin attributes for (host.*, telemetry.sdk.*)

[jmacd]

This adds resource.NewConfig(ctx, ...) with functional options:

• WithAttributes(...) for directly adding key:values
• WithDetectors(...) for directly adding resource detectors
• WithTelemetrySDK(...) to override the default telemetry.sdk.* resources
• WithHost(...) to override the default host.* resources
• WithFromEnv(...) to override the default environment configuration
• WithoutBuiltin() to disable all builtin resources

The intention is to make it easy to configure a *Resource with the standard resources. This adds standard resource detectors for the telemetry SDK and host name.

Resolves #1076.

[jmacd]

Resolves #1076

[codecov]

Codecov Report

Merging #1235 into master will increase coverage by 0.2%.
The diff coverage is 95.1%.

@@           Coverage Diff            @@
##           master   #1235     +/-   ##
========================================
+ Coverage    76.8%   77.1%   +0.2%     
========================================
  Files         122     123      +1     
  Lines        5953    5985     +32     
========================================
+ Hits         4574    4616     +42     
+ Misses       1131    1121     -10     
  Partials      248     248             

Impacted Files	Coverage Δ
sdk/resource/config.go	91.3% <91.3%> (ø)
sdk/metric/controller/pull/pull.go	100.0% <100.0%> (ø)
sdk/metric/controller/push/push.go	96.2% <100.0%> (ø)
sdk/metric/sdk.go	79.3% <100.0%> (-0.3%)	⬇️
sdk/resource/auto.go	100.0% <100.0%> (+100.0%)	⬆️
sdk/resource/builtin.go	100.0% <100.0%> (ø)
sdk/resource/env.go	100.0% <100.0%> (ø)
sdk/resource/resource.go	57.5% <100.0%> (ø)
codes/codes.go	89.6% <0.0%> (ø)
sdk/opentelemetry.go	100.0% <0.0%> (ø)
... and 3 more

[jmacd]

FYI I will address the feedback in this PR today.

[MrAlias]

We can probably merge this as is. Adding the exported type/method docs would be nice, but could be added in a follow on.

[jmacd]

I will document this PR today. Sorry @MrAlias, and thanks for keeping me honest.

[jmacd]

@open-telemetry/go-approvers how would you feel about removing the current resource.New(...label.KeyValue) *Resource and renaming this PR's NewConfig() as the replacement API, i.e., resource.New(context.Context, ...Option) (*Resource, error)? I think this will encourage the use of the detector pattern.

[MrAlias]

@open-telemetry/go-approvers how would you feel about removing the current resource.New(...label.KeyValue) *Resource and renaming this PR's NewConfig() as the replacement API, i.e., resource.New(context.Context, ...Option) (*Resource, error)? I think this will encourage the use of the detector pattern.

I am in favor of this 👍

[XSAM]

how would you feel about removing the current resource.New(...label.KeyValue) *Resource and renaming this PR's NewConfig() as the replacement API, i.e., resource.New(context.Context, ...Option) (*Resource, error)? I think this will encourage the use of the detector pattern.

I'm not sure about this. New(kvs ...label.KeyValue) *Resource seems quite simple to me. Also, does any chance that resource.New might return an error?

[jmacd]

I'm not sure about this.

I had originally used Configure() as the name, trying to make it clear that Configure() had some logic (possible errors) whereas New() just builds a resource (no errors). I hope that most end users will call only a single method, is the thing added in this PR, and the current New() method will mostly be used by detector implementations. Whatever we do, we should choose the right name from the majority of users' perspective.

[Aneurysm9]

@open-telemetry/go-approvers how would you feel about removing the current resource.New(...label.KeyValue) *Resource and renaming this PR's NewConfig() as the replacement API, i.e., resource.New(context.Context, ...Option) (*Resource, error)? I think this will encourage the use of the detector pattern.

I think that I like this approach. The addition of resource.WithAttributes() seems like a pretty small overhead for a more flexible API that may also encourage the use of detectors.

[jmacd]

I'd like this to make progress. The current ideas for this signature:

• NewConfig
• Configure
• New (where existing New is renamed something else).

Another proposal is to change the signature of Detect() to support the options in this PR. Most users, then, would only call Detect().

[Aneurysm9]

I think my order of preference would be:

• New (with existing New renamed or dropped)
• Detect
• NewConfig
• Configure

with the caveat that I for some reason actively dislike the last two. They just don't feel right compared with the other options.

[jmacd]

I applied your suggestion @Aneurysm9, renamed the new method to New() and renamed the old method to NewFromAttributes().

[MrAlias]

@XSAM we talked about this issue in the SIG meeting today and wanted to make sure we have consensus on this change going forward. We definitely value your opinion and want to understand if you are in strong opposition to these changes and if we can find an agreeable solution.

[XSAM]

@MrAlias I misunderstand what @jmacd is proposed. After reviewing the implementation, I believe this is the right decision.

[jmacd]

The only last thought I have here is maybe the new method should be NewWithAttributes() instead of NewFromAttributes(), since it's an abbreviation for New(..., WithAttributes(...), WithoutBuiltin()) that discards a nil error.

[MrAlias]

The only last thought I have here is maybe the new method should be NewWithAttributes() instead of NewFromAttributes(), since it's an abbreviation for New(..., WithAttributes(...), WithoutBuiltin()) that discards a nil error.

Yeah, that makes sense.

[MrAlias]

I think this is ready to merge. @jmacd @Aneurysm9 thoughts?

[jmacd]

I think this is ready! 😀