wemogy/libs-core

Adding Application Insights with an Empty String should be optional but allowed

Closed this issue · 4 comments

robinmanuelthiel commented

Currently, adding ApplicationInsights with an empty string fails.

.WithApplicationInsights("")

This is fine. But there should be an option to configure Application Insights optionally. Background for this is, that Application Insights locally can be slow, delay the start and might not be needed. Current workarounds run with if/else statements around this line. It would be nicer, if you could add it optionally.

Example:

https://github.com/wemogy/spaceblocks-permissions/blob/bb37bd5b85f577599f3340a626efd451636d34d3/src/webservices/main/SpaceBlocks.Permissions.WebServices.Main.Api/Startup.cs#L64C9-L69C10

Options:

  1. Allow adding an empty string to .WithApplicationInsights(string cs), log a warning when the string is empty and don't add App Insights then
  2. Introduce optional parameter .WithApplicationInsights(string cs, bool optional = false), so that the optional behaviour is clear
  3. Introduce new .WithOptionalApplicationInsights(string cs), so that the optional behaviour is clear

Reference

libs-core/src/Wemogy.Core/Monitoring/MonitoringEnvironment.cs

Line 52 in 5fa4b05

public MonitoringEnvironment WithApplicationInsights(string connectionString, float samplingRatio = 1f)

robinmanuelthiel commented

I heavily vote for Option 3). What do you think @SebastianKuesters?

SebastianKuesters commented

We could do option 3) but first let me show you, what we are doing in a real-world project right now:

// Add Monitoring
var monitoring = _options
    .AddMonitoring()
    .WithActivitySource(Observability.DefaultActivities.Name)
    .WithMeter(Observability.Meter.Name)
    .WithCQRS()
    .WithPrometheus();

var applicationInsightsConnectionString = Configuration["AzureApplicationInsightsConnectionString"];
if (!string.IsNullOrWhiteSpace(applicationInsightsConnectionString))
{
    Console.WriteLine("Application Insights is enabled.");
    monitoring.WithApplicationInsights(applicationInsightsConnectionString);
}

This solves the problem, but your WithOptionalApplicationInsights would avoid the if statement around which is nicer.

Looking forward for your feedback :)

robinmanuelthiel commented

Isn‘t this EXACLY what I showed (linked) under „Example“ in the issue description? 😅

SebastianKuesters commented

🤦 - we go with option 3

  • 👍1