-
Notifications
You must be signed in to change notification settings - Fork 358
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
Exposing App Insights endpoint configs #2376
Conversation
@cijothomas can you take a look at this as well? I know that App Insights is introducing (or has introduced?) connection strings with an endpoint suffix (microsoft/ApplicationInsights-dotnet#1221). Should we simplify this approach in functions and just expose the single suffix, or is it better to have the three separate properties like we have here? Eventually we should just support the connection string itself, but in V1 that likely won't happen and we're looking to unblock gov cloud connections ASAP. |
@@ -98,7 +98,7 @@ private IHost ConfigureHost(LogLevel minLevel = LogLevel.Information, HttpAutoCo | |||
}) | |||
.ConfigureServices(services => | |||
{ | |||
ServiceDescriptor quickPulse = services.Single(s => s.ImplementationType == typeof(QuickPulseTelemetryModule)); | |||
ServiceDescriptor quickPulse = services.Single(s => s.ServiceType.Name == nameof(QuickPulseTelemetryModule)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find a way to add the 3 AppInsights singletons with an ImplementationType and I don't think I fully understand the ramifications of adding them without one. This is the only thing I've found in the code that was counting on these modules having an ImplementationType, but I'm not sure if there are others.
@brettsam Can you sync with @TimothyMothra to see if using connectionstrings here make more sense than all the individual endpoints? |
@brettsam, If you have any questions about Connection Strings, feel free to message me. You may find it simpler to expose the Environment Variable This is available now in our SDK 2.12-Beta4. |
@gzuber @brettsam I want to politely ask that you consider onboarding to this version for Connection String support. |
@TimothyMothra thanks for the reminder! We are planning to use connection string for this feature, we've just been discussing internally the best way to expose it. I'll update this PR soon 😃 |
I actually ended up making a new branch as the changes for ConnectionString were more divergent than I expected. New PR is #2385 |
Resolves #2263
Initially made the decision to not group the endpoints because they're each configs for different components of App Insights. Left them at the top "Application Insights" level because giving them each their own subcategory seemed cluttered.
This would also require changes to the host.json schema