Google-cloud-dotnet: Enable configuration support for Google.Cloud.Diagnostics.AspNetCore

[tillig]

When using the Google.Cloud.Diagnostics.AspNetCore.GoogleDiagnosticsWebHostBuilderExtensions.UseGoogleDiagnostics extension, there is an assumption that the Google Cloud project ID is hardcoded. This is seen in the examples:

string projectId = "[Google Cloud Platform project ID]";
var webHost = new WebHostBuilder()
    .UseGoogleDiagnostics(projectId)
    .UseStartup<Startup>()
    .Build();

Looking at the extension, it appears this roughly boils down to a call to IWebHostBuilder.ConfigureServices(Action<IServiceCollection>) where several things get registered with DI.

There is another overload IWebHostBuilder.ConfigureServices(Action<WebHostBuilderContext, IServiceCollection>) where the web host can provide configuration for the app via WebHostBuilderContext.Configuration. This allows configuration to be set up then passed in to drive later registrations.

It would be helpful if there was a way to use this overload to avoid having to set up configuration multiple times (one for use in GCP calls, one for the actual app) and avoid hard coding.

For example, something like this:

public static IWebHostBuilder UseGoogleDiagnostics(this IWebHostBuilder builder, string projectIdKey = null, string serviceNameKey = null, string serviceVersionKey = null)
{
  builder.ConfigureServices((context, services) =>
  {
    var projectId = projectIdKey == null ? null : context.Configuration[projectIdKey];
    var serviceName = serviceNameKey == null ? null : context.Configuration[serviceNameKey];
    var serviceVersion = serviceVersionKey == null ? null : context.Configuration[serviceVersionKey];
    // then register the stuff as usual.
  }

  return builder;
}

A workaround would be to allow the GoogleDiagnosticsStartupFilter class to be public so folks could effectively write this overload as an app-specific extension. Unfortunately, with the GoogleDiagnosticsStartupFilter being internal, one can't actually write a custom version of the extension in a very clean fashion.

[iantalarico]

CC: @henkmollema who did the initial work here.

[henkmollema]

The intention behind this extension method is to retrieve the project ID (and service name/version) from the GCP resource where the application is deployed to. Calling UseGoogleDiagnostics() without parameters will just work when deployed to Google Cloud. Specifying the values explicitly is merely for testing and overriding. At least, that was _my_ intention.

I wonder what your scenario is that requires a dynamic or configurable project ID. Is it some kind of multi-tenant app?

Another way is to fall back to configuring the diagnostic services (logging, error reporting and tracing) individually:

builder.ConfigureServices((context, services) =>
{
    // Populate from context.Configuration
    services.AddGoogleExceptionLogging(options =>
    {
        options.ProjectId = projectId;
        options.ServiceName = serviceName;
        options.Version = version;
    });
}

And a question: how would your proposed overload play with the existing one?

UseGoogleDiagnostics(this IWebHostBuilder builder, string projectId = null, string serviceName = null, string serviceVersion = null)

If you add a config key parameter for project ID, service name and service it might become confusing

[tillig]

I'm really just getting started trying to use this and found that having something hardcoded isn't great. We have a diverse set of teams with more than one cloud project, so each team being able to override what the dev/test project is has value. Especially as we try to create some boilerplate app setup libraries that get shared across teams.

We are working on a system that is primarily a set of SaaS microservices, but larger customers can opt to deploy the services to their own cloud account. Still working out the kinks, trying to ensure that we can support that as we prove it out. Services are hosted in Kubernetes, so I'm not totally sure where "service name" or "service version" will be coming from in that scenario. Pod name? Deployment name? In any case, being able to control that is valuable.

I absolutely can configure all of that individually and that's effectively what I'm falling back to now with the current setup. It just seemed like a fairly easy thing to enable that might add value.

Apologies if the overload I posted wasn't entirely clear. It was a suggestion/idea rather than a pull request. Something to build on. Thinking more about it, maybe something more like this would be feasible:

public static IWebHostBuilder UseConfiguredGoogleDiagnostics(this IWebHostBuilder builder, string projectConfigurationKey = "google")
{
  builder.ConfigureServices((context, services) =>
  {
    var projectId = context.Configuration[$"{projectConfigurationKey}:project"];
    var serviceName = context.Configuration[$"{projectConfigurationKey}:serviceName"];
    var serviceVersion = context.Configuration[$"{projectConfigurationKey}:serviceVersion"];
    // then register the stuff as usual.
  }

  return builder;
}

In this case...

• A different name disambiguates the method overloads to avoid clashing and clarify intention.
• Only a root configuration key is required and the child keys under that are assumed. This is a precedent seen with the Microsoft.Extensions.Logging configuration where you can provide a root log configuration key but the child keys are assumed and not changeable.

Granted, the approach where each configuration key is provided individually would also work in combination with the updated method name; this would just provide some defaults so technically you could do UseConfiguredGoogleDiagnostics() with appSettings.json like:

{
  "google": {
    "project": "abcd-123456",
    "serviceName": "my-microservice",
    "serviceVersion": "1.2.3"
  }
}

That should, with the default web host builder, "just work." I'm working on a local version of pretty much this exact thing. The stumbling block is that I'll have to make my own GoogleDiagnosticsStartupFilter to allow things to be registered from the web host level (like UseGoogleDiagnostics()) without sprinkling code throughout the app startup as well.

Does that help clarify? Am I off base in my thinking somewhere?

[tillig]

I think part of what I'm running into is that I'm not running on Google App Engine. It appears that's the only platform that does allow the simple call to UseGoogleDiagnostics() to just work.

With UseGoogleDiagnostics() the project name, service name, and service version are retrieved by passing a null instance of MonitoredResource to various methods on the Project class that retrieve data from the running app.

The methods on the Project class look for properties based on what's available in the current platform:

• GetAndCheckProjectId looks for the project_id property.
• GetAndCheckServiceName looks for the module_id property.
• GetAndCheckServiceVersion looks for the version_id property.

If any of these is not supplied, it falls back to creating a MonitoredResource based on the current platform. If the value still isn't supplied, you get an exception.

The only platform type in MonitoredResource that supplies all three project_id, module_id, and version_id is Google App Engine.

| PlatformType | project_id | module_id | version_id |
| --- | --- | --- | --- |
| PlatformType.Unknown | ❌ | ❌ | ❌ |
| PlatformType.Gce | ✔️ | ❌ | ❌ |
| PlatformType.Gae | ✔️ | ✔️ | ✔️ |
| PlatformType.Gke | ✔️ | ❌ | ❌ |

So if I'm in a VM (GCE) or in Kubernetes (GKE) I have to provide data myself. Which isn't a big deal, to be sure, but means it'll happen whether it's in development or not, making configuration somewhat interesting.

[amanda-tarafa]

@tillig g I'll get a look at this and get back on this thread

[tillig]

If it would help, I would be happy to submit a PR (even if you don't take it and just want to see the code) for the bits I actually have working in GKE against .NET Core config.

[amanda-tarafa]

@tillig I've been looking at the code and both yours and @henkmollema's comments, and here are some comments of my own:

• The UseGoogleDiagnostics() extension method was meant as a helper method for configuring all of Diagnostics at once and cleanly when either running on GAE or running on GCE/GKE and having fixed (hardcoded) serviceName and version or running outside of Google Cloud and having projectID, serviceName and version fixed.
• As @henkmollema recommended, you can always fallback to configuring each of the 3 services individually on the Startup class where you have access to the Configuration property.
• This is definitely not clear in the docs, I will submit a PR to update the docs.
• I also definitely see the advantage of having a more general extension method for configuring all in one go that works no matter where the application is running or whether some logic is required to obtain the three values.
• I think that assuming the values will always be stored in appSettings.json, although very reasonable, might not be general enough.
• What about something like:

public static IWebHostBuilder UseGoogleDiagnostics(
    this IWebHostBuilder builder,
    Func<WebHostBuilderContext, string> projectIdGetter = null,
    Func<WebHostBuilderContext, string> serviceNameGetter = null,
    Func<WebHostBuilderContext, string> versionGetter = null)

• If you are happy to work on a PR like that, I'm more than happy to review and help with it.
• Else, I might be able to work on it, but need to confirm with the team first and probably can't guarantee an ETA.
• And one last alternative, can't you pass the serviceName and version values as command line arguments and use them from there?

[tillig]

I think the proposed overload is fine. Flexible enough to get the information from different places if needed; easy enough to wrap with a more convenient call with custom code.

I am working through the legal portion of getting the CLA signed. After I have that done I'd be happy to work on the PR for that.

Ostensibly I could get the values from command line args, the environment, or anywhere. We end up rolling all that together in IConfiguration anyway. I just need the flexibility to provide the values from a variety of sources (JSON config, XML config, environment, command line, whatever) and having the ability to do all that work in one pipeline as the web host is built is valuable. The work is already being done to assemble the config, why not use it?

But, like I said, I'm happy to just own our convenience method, the minimum I need is the GoogleDiagnosticsStartupFilter to be public so I can use it.

I'll keep you posted on the CLA.

[amanda-tarafa]

That's great, thanks! Let me know if you need anything from my side before you are able to work on the PR.

[tillig]

I haven't abandoned this, just still working through legal. Sorry for the delay.

[amanda-tarafa]

No worries, and thanks for the update.

[amanda-tarafa]

Closed via #2741 . We'll be releasing probably by the end of next week.

[amanda-tarafa]

@tillig This is now included in release 3.0.0-beta08. Thanks!

[henkmollema]

@amanda-tarafa @tillig this new overload breaks usages of the method without parameters, e.g.:

builder.UseGoogleDiagnostics();

With the following error message:

The call is ambiguous between the following methods or properties: 
'GoogleDiagnosticsWebHostBuilderExtensions.UseGoogleDiagnostics(IWebHostBuilder, string, string, string)' 
and 
'GoogleDiagnosticsWebHostBuilderExtensions.UseGoogleDiagnostics(IWebHostBuilder, Func<WebHostBuilderContext, string>, Func<WebHostBuilderContext, string>, Func<WebHostBuilderContext, string>)'

It can be fixed by choosing explicitly for one or another overload, e.g. builder.UseGoogleDiagnostics(projectId: null);. I do think that's pretty unfortunate though. Is there any way to fix this?

[amanda-tarafa]

Let me take a closer look at that. I'll updata here.

[amanda-tarafa]

@henkmollema @tillig This effectively introduced a breaking change as described by @henkmollema comment. Sorry I missed that during review.

So, three options I can think of to fix this:

1. Rename the offending overload to something like UseGoogleDiagnosticsFromContext.
2. Have the parameters in the offending overload not be optional. Them being optional is just a matter of comfort in that when wanting some of these values to be null, i.e. projectId, the user doesn't need to pass a projectIdGetter, but functionally, that would be the same as to pass a projectIdGetter that returns null.
3. Something more complex albeit more general, like defining an interface IGoogleDiagnosticsConfig that would have properties for ProjectId, ServiceName, ServiceVersion. And then the offending overload would turn into UseGoogleDiagnostics(this IWebHostBuilder builder, Func<WebHostBuilderContext, IGoogleDiagnosticConfig> configGetter). This would also allow for other implementations of IGoogleDiagnosticsConfig that would obtain the values from other places.

I would go for number 3. It's just a little more work but it will give us flexibility in the future in case we need to add either other configuration parameters or other ways to obtain the configuration paramaters.
Let me know what you think, if this would be usefull for you etc. And I will submit the PR.

@jskeet if you could chime in as well that'd be great.

[henkmollema]

Either option 2 or 3 seems fine.

Regarding Func<WebHostBuilderContext, IGoogleDiagnosticConfig> of option 3: did you mean for the consumer to _return_ an instance of IGoogleDiagnosticConfig, or did you mean to use Action<WebHostBuilderContext, IGoogleDiagnosticConfig> so the consumer _gets_ an instance of IGoogleDiagnosticConfig which they can populate with values using WebHostBuilderContext?

[jskeet]

I think the "make the parameters required" is the way I'd prefer here.

[amanda-tarafa]

@henkmollema I meant for Func<WebHostBuilderContext, IGoogleDiagnosticConfig>, the method implementation would look like this:

        public static IWebHostBuilder UseGoogleDiagnostics(this IWebHostBuilder builder, Func<WebHostBuilderContext, IGoogleDiagnosticConfig> configGetter)
        {
            builder.ConfigureServices((context, services) =>
            {
                var config = configGetter(context);
                ConfigureGoogleDiagnosticsServices(services, config.ProjectId, config.ServiceName, config.ServiceVersion, null);
            });

            return builder;
        }

[amanda-tarafa]

I'll be submitting option number 2 then, we can revisit number 3 in the future if we really need it.

[amanda-tarafa]

I have submitted #2900. Take a look and feel free to leave comments.

[amanda-tarafa]

Closed via #2900 . Will update here when this is released.

[amanda-tarafa]

The fix for the breaking change is now released in 3.0.0-beta09.