Make new classes in v4 public to allow usage of other DI containers

[brental]

There are a number of new classes added in v4 of the libraries that are internal. Similar to #126 these classes should be made public to allow for the usage of other DI containers.

Known classes are (but there may be others):

• VariantServiceProvider
• DefaultHttpTargetingContextAccessor
• ApplicationInsightsHostedService
• ApplicationInsightsEventPublisher

It would also be good if this could also be kept in mind for future new classes that are added as well. This will ensure new versions of the libraries support other DI containers.

[brental]

@zhiyuanliang-ms I noticed you reacted to this a while back and also made the changes related to #126. Are the changes suggested in this issue likely to be approved? Happy to put in a PR for the changes if so.

[zhiyuanliang-ms]

Hi, @brental Thank you for reaching out.

From perspective of library design, we only want to expose classes that users may need to use. But it seems like users will never use this classes directly, we have public APIs to allow you to use them indirectly.

Do you have specific scenario to use these classes directly?

[brental]

@zhiyuanliang-ms I don't have a specific need to use the classes directly myself, but as part of #126 various classes that were previously internal were made public to allow for their usage with other DI containers. This was even though there were public APIs that allowed for their usage indirectly via the built in .NET service provider and a public interface and users wouldn't need to use them directly. Making them public allowed for their usage completely with third party DI containers without the reliance on the built in service provider.

It seems to follow that given that change was made for the classes in #126, it would make sense to do the same for new classes added in v4. Otherwise, some of the library can be registered with third party DI containers but other classes in the library cannot be registered with third party DI containers (without resorting to reflection). That means that consumers cannot completely use third party DI containers for this library, they must rely at least partially on implementations being provided by the built in .NET service provider. That seemed to be the goal of the changes made in #126 but I could be mistaken.

Simarly, if there is a difference between the classes that were made public in #126 and the classes above that mean they shouldn't be made public when the other classes were would you be able to elaborate on that difference?

[brental]

I think the FeatureManager and VariantServiceProvider classes are a good comparison. Both are classes with an interface(s) that form part of the public API, these being IFeatureManager / IVariantFeatureManager and IVariantServiceProvider respectively. Both these classes are ones that consumers of the library would use via their interfaces. However, the FeatureManager class can be used with third party DI containers where as VariantServiceProvider class cannot.

Before the changes in #126 the FeatureManager class could not be registered with third party DI containers due to being private, but was made public in those changes so it could be registered with third party DI containers. However, despite it also being a class that consumers of the library might want to use with third party DI containers the VariantServiceProvider class cannot be registered with third party DI containers due to it being internal. So, if FeatureManager was made public so it can be used with third party DI containers it seems like the same should be done for VariantServiceProvider as well. It is this kind of inconsistency that this issue is about.

I haven't looked at the other classes listed in the original description in detail but it might also be good if they could be registered with third party DI containers.

[zhiyuanliang-ms]

Thank you for the detailed explanation! @brental I will bring this to the team.

[zhiyuanliang-ms]

The ApplicationInsightsHostedService is trick that we learned from the OTEL .net sdk to make sure ApplicationInsightsEventPublisher will be initialized in the DI container as expected. And I also don't think ApplicationInsightsEventPublisher should be public, as its only usage is to add the activity listener. I think it probably makes sense to have VariantServiceProvider and DefaultHttpTargetingContextAccessor public.

[brental]

@zhiyuanliang-ms Has there been any update on this issue?

[zhiyuanliang-ms]

Hey, @brental Hope everything goes well. Thanks for reaching out!

VariantServiceProvider this is completely designed for .NET DI system. So, for other 3rd party DI framework, it don't think it should be used.

DefaultHttpTargetingContextAccessor It makes sense to change this to a sealed public class to allow it to be used under 3rd party DI framework.

ApplicationInsightsHostedService and ApplicationInsightsEventPublisher, we don't think we should expose these two classes.

we only want to expose classes that users may need to use. We are very cautious about exposing public APIs and do everything we can to avoid breaking changes. Adding public APIs increases the risk of introducing breaking changes. The implementation of the Application Insights integration may change in the future, and we don’t want to expose that. Moreover, we don’t expect users to use these APIs directly.

CC @jimmyca15

[rossgrambo]

DefaultHttpTargetingContextAccessor depends on HttpContext which is AspNetCore. It will not play well with another DI framework as far as I know.

[brental]

Thanks for responding to this @zhiyuanliang-ms @rossgrambo.

I understand the point about trying to limit the exposed public API. So, given I dont have a specific need for these to be public I am happy to close this issue without changes. I will leave it open for now as there is the open PR for DefaultHttpTargetingContextAccessor, but once that is resolved either way (again, I dont have a specific need for it to be public at this stage so if it doesnt go ahead it is no problem) I can close this.