Removed different types of Detectors for Resources.

[MadVikingGod]

This change simplifies different types of collectors into one list. The order of this list determines how they are applied. Defaults are applied when the user does not supply any detectors. To achieve default behavior and additional behavior a DefaultDetectors struct has been created.

This addresses usability concerns from #1690

Feedback request:
Does this simplify things too much?
Is this better than what we have?

Closes #1813

[codecov]

Codecov Report

Merging #1810 (c90ebc1) into main (f92a6d8) will decrease coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##            main   #1810     +/-   ##
=======================================
- Coverage   78.7%   78.7%   -0.1%     
=======================================
  Files        137     137             
  Lines       7373    7363     -10     
=======================================
- Hits        5806    5798      -8     
+ Misses      1322    1320      -2     
  Partials     245     245             

Impacted Files	Coverage Δ
sdk/resource/builtin.go	90.0% <ø> (ø)
sdk/resource/env.go	100.0% <ø> (ø)
sdk/resource/config.go	90.9% <100.0%> (+3.4%)	⬆️
sdk/resource/resource.go	66.1% <100.0%> (+1.8%)	⬆️
sdk/resource/auto.go	85.7% <0.0%> (-14.3%)	⬇️
sdk/trace/batch_span_processor.go	87.1% <0.0%> (+1.5%)	⬆️

[seh]

I still find it strange that the function name New means "pre-populated," and NewEmptyResource means "empty," but has to stutter to get there. This seems like a too-literal reading of the specification's prescription for certain detectors being built in.

I don't take that to mean that every resource needs to enjoy these detectors unless told otherwise. Instead, I take that to mean that every SDK has to include these detectors at minimum, so that they're available for use.

I'd prefer that resource.New start with an empty resource, and that we provide a WithBuiltins or WithBuiltinDetectors option to allow the caller to include those built-in detectors. If a caller forgets to use WithBuiltins or WithBuiltinDetectors, that's not a failure of the library.

[MrAlias]

I still find it strange that the function name New means "pre-populated," and NewEmptyResource means "empty," but has to stutter to get there. This seems like a too-literal reading of the specification's prescription for certain detectors being built in.

I don't take that to mean that every resource needs to enjoy these detectors unless told otherwise. Instead, I take that to mean that every SDK has to include these detectors at minimum, so that they're available for use.

I'd prefer that resource.New start with an empty resource, and that we provide a WithBuiltins or WithBuiltinDetectors option to allow the caller to include those built-in detectors. If a caller forgets to use WithBuiltins or WithBuiltinDetectors, that's not a failure of the library.

I see your point, and overall share your view on this naming strategy. It is hard to say definitively one way or another without user research what our users will want in the end. But, I think you are right that having New perform the way you describe it will match common Go practices and might be less foreign to other users. I'm find with switching to this.

I would like to know what @jmacd thinks?

[MadVikingGod]

I still find it strange that the function name New means "pre-populated," and NewEmptyResource means "empty," but has to stutter to get there. This seems like a too-literal reading of the specification's prescription for certain detectors being built in.

I don't take that to mean that every resource needs to enjoy these detectors unless told otherwise. Instead, I take that to mean that every SDK has to include these detectors at minimum, so that they're available for use.

I'd prefer that resource.New start with an empty resource, and that we provide a WithBuiltins or WithBuiltinDetectors option to allow the caller to include those built-in detectors. If a caller forgets to use WithBuiltins or WithBuiltinDetectors, that's not a failure of the library.

I see your point, and overall share your view on this naming strategy. It is hard to say definitively one way or another without user research what our users will want in the end. But, I think you are right that having New perform the way you describe it will match common Go practices and might be less foreign to other users. I'm find with switching to this.

I would like to know what @jmacd thinks?

I think this is the wrong approach. I would expect that most uses of a resource would be likely to include the built-in detectors. But, without any user stories (or probably more likely instrumentation libraries) I am willing to accept this.

[seh]

I would expect that most uses of a resource would be likely to include the built-in detectors. But, without any user stories (or probably more likely instrumentation libraries) I am willing to accept this.

Let's not give up on reaching accord. We have been through quite a journey on this topic.

Was there an earlier version where resource.New included the built-in detectors, but we had a WithoutBuiltins (WithoutBuiltinDetectors?) or similar functional option that could clear them? I proposed that in #1690 (comment). I apologize if I've since argued against that same idea.

[MadVikingGod]

Let's not give up on reaching accord. We have been through quite a journey on this topic.

I think we have reached an accord, just not a unamious one. We have two different paths one New() returns the empty resource, and the other New() returns a filled-out version. Both are valid ways for an "object" to be constructed in go.

The former is probably more common, but I think less useful in this case.
The latter is more useful I think because most use cases, instrumentation and client applications, will want to have the default detectors with maybe additional ones.

The problem is that without any use-case data it is just what I think.

If 3 out of the 4 people, including myself, who have commented so far came to the same conclusion then I am willing to commit to the majority decision. I don't think we can change this behavior after 1.0, but I think the worst thing that comes out of the current configuration is there is a lot of missing resource descriptions, and a lot of extra WithBuiltins() written.