serilog / serilog-enrichers-environment

Enrich Serilog log events with properties from System.Environment.
Apache License 2.0
80 stars 33 forks source link

Using hostsettings.json results in incorrect environment name #48

Open erikmf12 opened 3 years ago

erikmf12 commented 3 years ago

When using a hostsettings.json file to set the environment, only the built-in source context logs come through with the correct environment. Any custom code that calls an ILogger log method enriches the logs with the default value of "Production".

flennic commented 3 years ago

One issue seems to be that the implementation only looks into environment variables:

// Qualify as uncommon-path
[MethodImpl(MethodImplOptions.NoInlining)]
private static LogEventProperty CreateProperty(ILogEventPropertyFactory propertyFactory)
{
    var environmentName = Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT");

    if (string.IsNullOrWhiteSpace(environmentName))
    {
        environmentName = Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT");
    }

    if (string.IsNullOrWhiteSpace(environmentName))
    {
        environmentName = "Production";
    }

    return propertyFactory.CreateProperty(EnvironmentNamePropertyName, environmentName);
}

In that regard, the enricher is actually correct, it looks into the environment variable and, if not found, defaults to Production. This behaviour is probably expected in a console application, but not in the context of an asp.net application, where you have multiple built-in ways to control the environment variable, like:

See: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-5.0

Most people, including me, do not expect this behaviour when using an asp.net application, where you use IWebHostEnvironment to obtain the current environment, as it could be set from any source.

One solution would probably be to add another property like WebHostEnvironmentNamePropertyName. A completely new enricher like Serilog.Enrichers.WebHostEnvironment or something in that direction might also be an option.

I would like to hear the opinion of others.

erikmf12 commented 3 years ago

I think we need some solution for these different hosting models. I use the Worker Service template extensively for services and even though it's not an asp.net app, it still uses the same hosting model functions. We sometimes run dev and prod on the same machine for smaller services and the env vars are always set the same for both, so we use a hostsettings.json file to set the environment.

I think a good alternative would be adding another property named something like EnvironmentNameFromHost that specifies that it's getting the environment from the running IHost, whether it's a web host or a generic host.

flennic commented 3 years ago

Probably a good idea to have it as generic as possible, so I like your suggestion. The question is just how, or rather where, to implement it. I would also like a solution where it tries to get the IHost implementation and if that is not possible, it defaults to the current implementation.

erikmf12 commented 3 years ago

Would it be best to add this environment enricher into the serilog-extensions-hosting repo? I know most of these add-on enrichers come in their own repo but it might make sense to add this enricher directly in there since that will be the only time it's needed.

EDIT: I suggested this because if we add it there, we'll have access to the underlying hosting environment.

nblumhardt commented 2 years ago

Definitely seems like this method is in the wrong place 👍

Not certain how an implementation in Serilog.Extensions.Hosting would look, but interested to check it out if anyone can sketch one up.