Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AspNetCore AddApplicationInsightsSettings() and MissingMethodException #1702

Closed
TimothyMothra opened this issue Feb 21, 2020 · 2 comments
Closed
Assignees
Labels
Milestone

Comments

@TimothyMothra
Copy link
Member

This was brought to my attention by @brettsam


In AppInsights.AspNetCore 2.12.0 I changed the method signature of AddApplicationInsightsSettings by adding the optional parameter connectionString.

public static IConfigurationBuilder AddApplicationInsightsSettings(
this IConfigurationBuilder configurationSourceRoot,
bool? developerMode = null,
string endpointAddress = null,
string instrumentationKey = null,
string connectionString = null)

This can cause a MissingMethodException for users upgrading from an older version of our SDK.
(Explanation of MissingMethodException and default parameters: https://stackoverflow.com/a/9884700/1466768)

Next Steps

I think this will be a straightforward fix, I should be able to add a method overload matching the signature of the original method.
I'm setting up a repro right now so I can verify a fix.
The plan is to release a patch for versions 2.12, 2.13, and 2.14

Next Next Steps

This highlights the need to add a PublicApiAnalyzer to our other projects.
We currently only have this analyzer on 2/17 of our projects.

@TimothyMothra
Copy link
Member Author

TimothyMothra commented Feb 21, 2020

I have a repro and I'm verifying the fix.

I'm trying to keep both method signatures (old and new), but this can cause AmbiguousMatchException.

I think the solution is going to look like this:

        public static IConfigurationBuilder AddApplicationInsightsSettings(
            this IConfigurationBuilder configurationSourceRoot,  
            bool? developerMode = null, 
            string endpointAddress = null, 
            string instrumentationKey = null)
            => configurationSourceRoot.AddApplicationInsightsSettings(developerMode, endpointAddress, instrumentationKey);

        public static IConfigurationBuilder AddApplicationInsightsSettings(
            this IConfigurationBuilder configurationSourceRoot,
            string connectionString,
            bool? developerMode = null,
            string endpointAddress = null,
            string instrumentationKey = null)
        {

Note that connectionString can't be an optional parameter because that causes the Ambiguous exception.
The only fix is going to be moving it to the first parameter, which will be a breaking change for anybody who took a dependency on it being the last. :(
Anyone who used named parameters would be immune to this problem.
Fun times.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant