Clumsy resource.New options

[MrAlias]

The WithFromEnv, WithHost, and WithTelemetrySDK options that are passed to New all accept a Detector as an argument. This Detector is stated to be used as an alternate to the builtin Detector that finds the related resource attributes, or if nil will omit those resource attributes. This feels very clumsy.

For instance, there is not guarantee that a Detector is passed to WithTelemetrySDK that will detect telemetry.sdk.* attributes. It could detect whatever attributes it wants. The only meaningful effect including this option will have is that it will not include the builtin detector (nil or otherwise).

Based on this behavior, it seems like these options should be changed to Without* versions of themselves. If a user wants to provide a custom detector to override these attributes they can use the WithDetectors option.

Proposal

• Replace WithFromEnv with

  // WithoutFromEnv disables the builtin FromEnv detector from being run.
  func WithoutFromEnv() Option { /* ... */ }
  

  (though this might be removed entirely in The SDK MUST extract information from the OTEL_RESOURCE_ATTRIBUTES environment variable #1689)
• Replace WithHost with

  // WithoutHost disables the builtin Host detector from being run.
  func WithoutHost() Option { /* ... */ }
  

• Replace WithTelemetrySDK with

  // WithoutTelemetrySDK disables the builtin TelemetrySDK detector from being run.
  func WithoutTelemetrySDK() Option { /* ... */ }
  

[seh]

I think this approach won't age well, as we will have users relying on these functions that would later turn out to have no effect if we were to revise which things the default detectors handle automatically. In other words, say that we later decide that we shouldn't detect the host-related details automatically. We would have users calling WithoutHost, which would no longer have any effect, and we'd probably then need to introduce withHost again.

[MrAlias]

@seh what is your proposal?

[seh]

We have WithoutBuiltin, which takes care of clearing all of them. What if we recast the three above to not take a Detector parameter, and made them niladic instead? We could document the default behavior as being equivalent to having passed WithFromEnv(), WithHost(), and WithTelemetrySDK(). If you don't want all of them, pass WithoutBuiltin() followed by whichever of those three you'd like to add back in.

What I could possibly see someone griping about is not knowing all of things that are built in, but wanting to just knock one of them out—"all of them except this one that I know I don't want." It feels like a problem in want of a bitmask.

[MadVikingGod]

What if we remove the three WithFromEnv, WithHost, and WithTelemetrySDK and use the default ones unless WithDetectors is used.

To make it convenient to add your own detector and have the builtins, we could have an exported slice of BuiltinDetectors. The New call would then look like this:

resource.New(
    WithDetectors(BuiltinDetectors...),
    WithDetectors(MyCustomDetector),
)

[MadVikingGod]

@seh I put in a PR just toying around with this. Can you take a look and see if that format would be more approachable?

[MrAlias]

Closed with #1810