Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using hostsettings.json results in incorrect environment name #48

Open
erikmf12 opened this issue Oct 18, 2021 · 5 comments
Open

Using hostsettings.json results in incorrect environment name #48

erikmf12 opened this issue Oct 18, 2021 · 5 comments

Comments

@erikmf12
Copy link

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
Copy link

flennic commented Oct 25, 2021

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:

  • Settings files, such as appsettings.json
  • Environment variables
  • Azure Key Vault
  • Azure App Configuration
  • Command-line arguments
  • Custom providers, installed or created
  • Directory files
  • In-memory .NET objects

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
Copy link
Author

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
Copy link

flennic commented Oct 25, 2021

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
Copy link
Author

erikmf12 commented Oct 25, 2021

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants