Make it easy to not use Stackdriver diagnostics in development

[jskeet]

Often when developing an app, you really don't want all the logging to go to Stackdriver - you only want it locally.

That's easy enough to handle if you're in Startup.cs - you can use env.IsDevelopment() etc. But if you're just calling UseGoogleDiagnostics, that isn't an option.

It turns out we do have access to the hosting environment by the time we register the loggers etc, so this is easy enough to do - but it needs to be configurable. (If you're trying to diagnose why Stackdriver Logging isn't working, you really do want to log to Stackdriver!)
I believe this would end up being a UseGoogleDiagnostics-specific option, so I'm not sure how much sense it makes to put it into LoggerOptions etc.

Ideally, we wouldn't even validate that we can get a project ID until we knew we were going to need it. So by default you'd be able to write .UseGoogleDiagnostics() and it would just do nothing in development and work in staging/production.

All of this has got me to wondering whether adding optional LoggerOptions etc on UseGoogleDiagnostics was the right approach. @henkmollema mentioned this in a PR. I wonder whether instead we might want a GoogleDiagnosticsOptions which has:

• UseInDevelopment (true/false, default = false)
• RetryOptions
• BufferOptions
• LoggerOptions
• ErrorReportingOptions
• TraceOptions

The retry options and buffer options would only be used where service-options weren't specified, but they'd be handy for the Cloud Run case of "don't buffer anything" or the troubleshooting case of "don't buffer or retry anything".

Obviously this would be a breaking change compared with 3.0.0-beta14, but I think that's okay, particularly if we can get to it soon.

cc @SurferJeffAtGoogle - we've talked about this before
cc @henkmollema - would love to hear your views

[simonz130]

How about having a boolean environment variable, like STACKDRIVER_BACKEND_LOGGING_OVERRIDE?

And the behavior would be:

• env.IsDevelopment() == true && STACKDRIVER_BACKEND_LOGGING_OVERRIDE not set => Log locally.
• env.IsDevelopment() == true && STACKDRIVER_BACKEND_LOGGING_OVERRIDE set to FALSE => Log locally
• env.IsDevelopment() == true && STACKDRIVER_BACKEND_LOGGING_OVERRIDE set to TRUE => Log into Stackdriver.
• env.IsDevelopment() == false => log into Stackdriver - irrespective of the state of STACKDRIVER_BACKEND_LOGGING_OVERRIDE.

This can apply to tracing and error reporting in the same way.

[jskeet]

I'd prefer to have it in the options. Environment variables feel a little too invisible to me. Is there a specific benefit you're thinking of?

[simonz130]

Yes. The goal (in my opinion) is to reach the state of "write once, run everywhere" - The same code works without developer changes in the code . It's the config that drives the decision of in which environment the code is executed.
If we go with the options solution, one needs to create two options objects - one that works for PROD and one that works locally. When developer tests this code, they will have to introduce "if" logic to distinguish between prod and local so both paths can be tested. Often times it might lead to introducing a boolean configuration value for the purpose of distinguishing the environments. With the environment variable solution, the complexity is encapsulated in platform implementation (i.e. our diagnostics library). By tweaking environment variable, developer can test the same code working under different "environments". This is also in the spirit of tools like GCLOUD that rely on environment variables like GOOGLE_APPLICATION_CREDENTIALS.

Curious to hear more thoughts on this.

[jskeet]

No need for any code if the user doesn't want to have it:

• We'd already be hooking into the environment part, so usually the developer wouldn't want to do anything anyway. They'd only want to log to Stackdriver when running locally for testing purposes, which would be a reasonable code change anyway. (It would be easier to change that bit of code than change their launch settings to change the environment variable.)
• They could potentially load the settings from appsettings.json, which can even take environment variables into account anyway :) Although it's possible that we'd be configuring this earlier in the process than when the settings are loaded.

[jskeet]

Note that a GoogleDiagnosticsOptions might also have an Action<TraceServiceOptions> to allow configuration of that (which is ASP.NET Core specific, unlike TraceOptions).

[amanda-tarafa]

@henkmollema hi! Any thoughts on this? I might start working on it soon and would like to hear your take on it.

As per the environment variable vs. options, my two cents here. I tend to prefer the options as well, I see env variables as "too decoupled" from development. And:

• You can't have 2 different applications running on the same environment with different values of the env variable. I know this might be less and less common now, but it's still done.
• Some people don't even have permissions to add or change env variables, think school computers and some workplaces.
• I wouldn't compare configuring of a tool, as GCLOUD, with configuring a client library, I don't think these need or should be in the same spirit?
• @jskeet's suggestions that clients could potentially load settings from appsettings.json, which take env variables in as well, is valid. We already added an overload of UseGoogleDiagnostics that takes the project ID, service name, etc. from the WebHostBuilderContext, which allows access to env variables, appsettings.json, command line arguments, etc. If we add GoogleDiagnosticsOptions we could always add an overload of UseGoogleDiagnostics that takes in a Func<WebHostBuilderContext, GoogleDiagnosticsOptions> optionsGetter to give all the flexibility to the client. See the discussion on Enable configuration support for Google.Cloud.Diagnostics.AspNetCore #2435 and the accompanying PR [Diagnostics] Fixing a breaking change introduced in #2741. #2900 for more context.

[jskeet]

Closing having moved this to the backlog.