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

Added WithEnvironmentName enricher #42

Merged
merged 3 commits into from
Dec 12, 2020
Merged

Added WithEnvironmentName enricher #42

merged 3 commits into from
Dec 12, 2020

Conversation

Nevets82
Copy link
Contributor

@Nevets82 Nevets82 commented Dec 8, 2020

Added support for 'EnvironmentName' property based on DOTNET_ENVIRONMENT or ASPNETCORE_ENVIRONMENT environment variable. Basically adds 'Production', 'Staging', 'Development', ... to the event logs.

Added support for 'EnvironmentName' property based on `DOTNET_ENVIRONMENT` or `ASPNETCORE_ENVIRONMENT` environment variable.Basically adds 'Production', 'Staging', 'Development', ... to the event logs.
@Numpsy
Copy link
Member

Numpsy commented Dec 8, 2020

should it be adding the property in the case where neither environment variable is set?

@Nevets82
Copy link
Contributor Author

Nevets82 commented Dec 9, 2020

We should probably add the property with value 'Production, as this seems to be the default case in the AspNetCore documentation and HostBuilder.CreateHostingEnvironment method.

Set EnvironmentName fallback value  to Production.
@nblumhardt
Copy link
Member

This seems reasonable based one the current contents of this library 👍

I wonder though if, in the bigger picture, we're not adding a lot here now that can't be written more obviously as variations of:

.Enrich.WithProperty("EnvironmentName", Environment.GetEnvironmentVariable("DOTNET_ENVIRONMENT") ?? "Production")

I notice that in the ASP.NET Core docs, it mentions that ASPNETCORE_ENVIRONMENT overrides the DOTNET_ENVIRONMENT - perhaps we should follow the same logic here?

@Numpsy
Copy link
Member

Numpsy commented Dec 11, 2020

Think that's what #24 was getting at?

@Nevets82
Copy link
Contributor Author

The reason why I would add this to this package, is because it then can also be configured by adding WithEnvironmentName to the Enrich property in the serilog-settings-configuration.

The difference I see with #24 is that this is more a framework environment variable and #24 is a good solution for custom environment variables.

Concerning the order of ASPNETCORE_ENVIRONMENT or DOTNET_ENVIRONMENT, I added DOTNET_ENVIRONMENT first because this seems to be the new standard (since verion 3.0) and ASPNETCORE_ENVIRONMENT is in my opinion the default for backwards compatibility. But this can deffinitely be changed.

@nblumhardt
Copy link
Member

👍 thanks for the reply.

I think we should follow the precedence the framework uses, since it would be confusing for the log to report e.g. "Production" while the app, using the variables in reverse order, might be in "Development"

https://docs.microsoft.com/en-us/aspnet/core/fundamentals/environments?view=aspnetcore-5.0#environments is the doc:

The ASPNETCORE_ENVIRONMENT value overrides DOTNET_ENVIRONMENT.

Otherwise LGTM!

The 'ASPNETCORE_ENVIRONMENT' value overrides 'DOTNET_ENVIRONMENT'.
@Nevets82
Copy link
Contributor Author

OK, I concur. I changed the precedence to ASPNETCORE_ENVIRONMENT.

@nblumhardt nblumhardt merged commit 24c1c87 into serilog:dev Dec 12, 2020
@nblumhardt
Copy link
Member

👍

@nblumhardt nblumhardt mentioned this pull request Aug 15, 2021
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

Successfully merging this pull request may close these issues.

3 participants