Add Commented Out Code for Azure Native Monitoring #464
Conversation
src/eShop.AppHost/Program.cs
Outdated
| @@ -18,9 +18,14 @@ | |||
|
|
|||
| var launchProfileName = ShouldUseHttpForEndpoints() ? "http" : "https"; | |||
|
|
|||
| // To configure Azure Monitor Application Insights, add the following: | |||
| // var insights = builder.AddConnectionString("myInsightsResource", "APPLICATIONINSIGHTS_CONNECTION_STRING"); | |||
There was a problem hiding this comment.
This can be replaced with a conditional such as
var appInsights = (builder.Configuration.GetConnectionString("myAppInsightsResource") is not null)?
builder.AddConnectionString(
"myAppInsightsResource",
"APPLICATIONINSIGHTS_CONNECTION_STRING")
: null;
src/eShop.AppHost/Program.cs
Outdated
| // Services | ||
| var identityApi = builder.AddProject<Projects.Identity_API>("identity-api", launchProfileName) | ||
| .WithExternalHttpEndpoints() | ||
| // .WithReference(insights) // Sample reference to Azure Monitor Application Insights. Copy/paste for each service. |
There was a problem hiding this comment.
I was going to add an overload to have a conditional reference, but it already seems to be there, so the following could be added to each app and not need to be commented out. It should silently ignore it if the resource is null.
.WithReference(appInsights,null,true);@mitchdenny - It turns out this doesn't work as the resource parameter isn't nullable. Is there a cleaner workaround?
There was a problem hiding this comment.
Yeah the .WithReference doesn't like that appInsights could be null.
There was a problem hiding this comment.
Why not just use a conditional? I'm not sure we want to invent a new API for this?
There was a problem hiding this comment.
@mitchdenny conditionals don't fit nicely into the chained builder pattern, unless there is something I don't know?
|
@dotnet-policy-service agree company="Microsoft" |
|
@samsp-msft Thoughts on my latest push? I added conditionals to each service reference. |
If necessary, we can do that, but I really would prefer an ability to do a conditional cleanly in the apphost as its going to be a pretty common scenario. @mitchdenny - How can we make conditionals clean in the apphost file? |
|
We met and discussed the options - nothing is available right now that makes this cleaner. we should go in with the commented out blocks, and @mitchdenny will file an issue for adding a conditional in the builder pattern. |
|
Issue: dotnet/aspire#5192 |
|
Azure specific components are included in eShopOnAzure. AppInsights (Azure Monitoring) is already included in eShopOnAzure: https://github.com/Azure-Samples/eShopOnAzure/blob/9da1eccec3454b19cd11a9b44001cd0323deb749/src/eShop.AppHost/Program.cs#L10 |
|
Great to see this on eShopOnAzure!! I tested it and got it to run...but ran into a few issues along the way.
Also, I needed to add a Connection String to appsettings.json. Should we document that somewhere? |
No description provided.