Add simpler DI-enabled `AddDetector<TResourceDetector>()` overload

[julealgon]

Feature Request

Today, there are a couple of overloads for the ResourceBuilder.AddDetector method, namely:

ResourceBuilder AddDetector(IResourceDetector)

ResourceBuilder AddDetector(Func<IServiceProvider, IResourceDetector>)

Is your feature request related to a problem?

I believe the current interface is too verbose and beginner-unfriendly for any resource detector that needs container dependencies to work.

Describe the solution you'd like:

I propose a third overload of the form:

ResourceBuilder AddDetector<TResourceDetector>() 
    where TResourceDetector : IResourceDetector

This overload would automatically resolve the detector from the container without needing an explicit factory like the one above it.

They can also be built on top of each other:

public static ResourceBuilder AddDetector<TResourceDetector>(this ResourceBuilder resourceBuilder)
    where TResourceDetector : IResourceDetector
{
    ArgumentNullException.ThrowIfNull(resourceBuilder);

    return resourceBuilder.AddDetector(p => ActivatorUtilities.GetServiceOrCreateInstance<TResourceDetector>(p));
}

The use of ActivatorUtilities.GetServiceOrCreateInstance allows consumers to either rely on the container to build-up the instance with container-driven dependencies, or register the detector explicitly separately and rely on the container to create the instance.

Describe alternatives you've considered.

I've actually created the extension method above in our own codebase for this exact reason, and I'd rather it existed in the library itself.

Additional Context

This same pattern is already in use throughout the framework. Take AspNetCore middlewares, for example, or the Azure Functions framework. They both support a similar pattern where the instance can optionally be created by the container using generics syntax.

I believe this change is thus not only helpful to streamline the API, but also consistent with the rest of the ecosystem.

I'd also go as far as to say that the Func<IServiceProvider, IResourceDetector> overload could be dropped from the API in favor of the new one as it will cover the vast majority of DI cases, and those that aren't covered, can rely on a factory registration on the container to get the same behavior. I don't necessarily consider this a part of this request however, just something to keep in mind.

[martinjt]

I'd say this is a pretty sane addition to the API and adds some consistency.

Would you be willing to do a PR for it?

What are your thoughts @CodeBlanch ?

[CodeBlanch]

I definitely agree with the API and would love to have it!

I don't know if the implementation is the correct direction though. The reason we don't already have this API is ResourceBuilder shipped long before we had any DI support. Users can do this today...

        ResourceBuilder.CreateEmpty()
            .AddDetector(sp => sp.GetRequiredService<IResourceDetector>())
            .Build();

That will result in a NotSupportedException being thrown because that ResourceBuilder is created on the fly is not attached to any IServiceCollection \ IServiceProvider.

The suggested implementation is using ActivatorUtilities.GetServiceOrCreateInstance to get around that. It will just spit back something as if everything was happy. Could be fine. But that is very different from everything we have today and the inconsistency worries me.

I'm sure this is solvable in a different way but there doesn't seem to be much demand for this API so I haven't spent a lot of time on it. For the most part it seems users are happy with the current shape.

What I would prefer to do is leave this out here to see if it gets a lot of 👍 before we rush into anything.

[martinjt]

I think if someone is willing to put in the work to do it properly I think that's fine and it should be accepted, but likely not something anyone can divert attention to as you say.

I think you're right on the implementation, doesn't sound like the right way to do it.

[julealgon]

@CodeBlanch I'm a bit confused by your answer here.

I definitely agree with the API and would love to have it!

I don't know if the implementation is the correct direction though. The reason we don't already have this API is ResourceBuilder shipped long before we had any DI support. Users can do this today...

        ResourceBuilder.CreateEmpty()
            .AddDetector(sp => sp.GetRequiredService<IResourceDetector>())
            .Build();

That will result in a NotSupportedException being thrown because that ResourceBuilder is created on the fly is not attached to any IServiceCollection \ IServiceProvider.

All of that happens today irrespective of my proposed changes, so I fail to see how an additional overload that builds on top of it would somehow make it worse? Can you elaborate? Are you implying the IServiceProvider-based overload should not be there in the first place?

Again... I'm confused here.

The suggested implementation is using ActivatorUtilities.GetServiceOrCreateInstance to get around that. It will just spit back something as if everything was happy. Could be fine. But that is very different from everything we have today and the inconsistency worries me.

I didn't use ActivatorUtilities.GetServiceOrCreateInstance in the proposal to get around anything, actually... The reason I opted for that specific method is that it is the most standard approach to tap into IServiceProvider while supporting both registered and unregistered instances. As I mentioned, this is already how it works on areas such as Azure Functions: you can register the function explicitly, or you can register just its dependencies. Whatever you do, the factory respects it by using this.

The issue you describe is related to lack of a IServiceProvider, so whatever error happens, happens the same way with my method. Unless I'm missing something very obvious here:

...are you perhaps confusing ActivatorUtilities class, from Microsoft.Extensions.DependencyInjection.Abstractions, with the standard reflection-based Activator class?! That`s the only thing that comes to my mind that would explain the concerns you described.

I'm sure this is solvable in a different way...

I'd be curious to hear what you have in mind about that.

...but there doesn't seem to be much demand for this API so I haven't spent a lot of time on it. For the most part it seems users are happy with the current shape.

That's a shame really. Any resource detector that needs dependencies becomes a verbose mess to create IMHO.

What I would prefer to do is leave this out here to see if it gets a lot of 👍 before we rush into anything.

Fair enough. For now, I'll publish this method as part of one of our internal libraries then.

To be honest, I wasn't expecting there to be resistance against what (to me) is such an obvious improvement in usability.

---

@martinjt

I think if someone is willing to put in the work to do it properly I think that's fine and it should be accepted...

Can you elaborate what a proper implementation would look like?

[martinjt]

My suggestion was something thar was more holistic as a change, as was suggested.

I think, if things are being misunderstood as your suggestion, the best option maybe to submit a PR that we can review. Perhaps that will be sufficient.

What we need to consider as a project as that the SDK needs to be able to be used without an external DI mechanism, as well as using the internal DI in the provider too. So if that just works, then maybe it's fine. Without seeing the actual code change suggestion though, I don't see a way forward immediately.

In summary, the API addition is solid. We should do it, but no-one who is currently working on the project will be able to spend time on it. However, we do take contributions seriously, and I'll happily help you work that through if you want to give it a go.

[julealgon]

Fair enough @martinjt . I thought I had provided all that was needed to fully understand this, but I'll send a PR later with the change.