Change the default ConsoleFormatter to Json when running in a container #2725
Comments
|
Tagging subscribers to this area: @maryamariyan Issue DetailsCurrently, the console logger in Some limitations of the current formatter:
What I would like to do is to switch the default ProposalUse the
Questions
Unless folks opt-in to using ILogger there’s no way to make this change to all images. We can do this for all hosted (ASP.NET/worker) app models. If you chose to Console.WriteLine directly, there’s not much we can (or should) do.
Possibly? That’s why I was hoping to solicit feedback before making the change.
Looking at the common deployment targets for hosted app models:
Of the top four most common deployment targets, it seems like Linux containers are the only scenario where defaulting to json makes sense.
There’s no impact here to other processes in a container. The change we’re discussing only impacts the output of hosted applications. If you do happen to build and run in an SDK container, then yes I expect the console logging from your hosted application to be JSON formatted. 👀 @maryamariyan @richlander @davidfowl @noahfalk
|
Is this the only case where folks want a different default logger? Seems like correlation rather than causation. Would it make more sense to do this whenever the console is redirected?
If they can set an environment variable, why not just set the logger this way? Goes back to the correlation question. |
It may not be possible to tell. If I'm writing to stdout in Docker, there's no way to tell what logging driver is used and whether it's being redirected.
This environment variable is already baked into all container images we (dotnet) produce. See- https://github.com/dotnet/dotnet-docker/blob/master/src/runtime-deps/3.1/alpine3.12/amd64/Dockerfile#L18 My less preferred option is to detect whether we're in a container using something like what we have below: public static bool IsInDockerContainer
{
get
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
{
// Check if one of the control groups of this process is owned by docker
if (File.ReadAllText("/proc/self/cgroup").Contains("/docker/"))
{
return true;
}
// Most of the control groups are owned by "kubepods" when running in kubernetes;
// Check for docker environment file
return File.Exists("/.dockerenv");
}
// TODO: Add detection for other platforms
return false;
}
} |
|
@maryamariyan I would rather we do this in an earlier preview since this is a breaking behavior change |
|
As a third option, we could invent a new variable for this. There is some prior art for using a new variable- https://github.com/dotnet/runtime/blob/79ae74f5ca5c8a6fe3a48935e85bd7374959c570/src/libraries/Microsoft.Extensions.FileProviders.Physical/src/PhysicalFileProvider.cs#L24 |
|
For sake of completeness, I should also mention that aspnetcore already uses the existing |
That's what I was getting at with
I'm fine either way, but trying to think about whether all customers who set Are there any other customers who want this behavior but don't want to set |
Be more explicit in their code about which logger formatter they want to use.
Possibly. I tried to answer that with the FAQ in my issue, but I'll repost it here 😄 Looking at the common deployment targets for hosted app models:
Of the top four most common deployment targets, it seems like Linux containers are the only scenario where defaulting to json makes sense. |
Seems like there is agreement on taking this approach |
|
So the fix plan for this issue here is to have dotnet docker images take environment variable |
|
I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label. |
|
[Triage] This was transferred to dotnet-docker as a result of the discussion in dotnet/runtime#50812. To begin with we are proposing to set This change should be made in 6.0 onward. It would be a breaking change to make in released versions. |
mthalman
added
the
needs-announcement
|
Is there a way how to unset the variable? Using |
|
Seems like we should revert this change. It's causing more pain then good. We can document the switch to JSON so people can put it in their containers if they want to. |
|
While not a big issue for me, it looks a little weird when every other container I'm using is outputting human readable logs (redis/postgres/haproxy etc) and AspNetCore app just dumps Json. |
This is what I thought too. If I open any of my k8s clusters and look at the logs of any of the system/plugin pods - none of them are outputting JSON. So it's making .NET quite inconsistent, even in the container/k8s world. |
Agreed : ) I think #2748 is the relevant change to revert. |
|
I'm open to reverting for .NET 7 but not necessarily for .NET 6. This change in behavior was already announced as part of the .NET 6 release. Changing it in a servicing release will introduce another wave of feedback from people affected by the change. There was a reason this change was made, right? What's the impact to the UX if the formatter is defaulted back to console? I'd like to see a thorough analysis from the diagnostics team explaining what the state of things should look like and how it impacts the UX. |
Reverting this will impact customers who have taken a dependency on the JSON format. Does anyone have a sense of how many this is? Our stated policy is to not make breaking changes to the images during servicing releases. If we do decide to make an exception, we will need to broadcast this (e.g. post announcements, include in release notes, etc.). Adding an image variant with the human readable formatter is another option to consider. This has issues as well but is an option. |
IMHO this should never have been the case. This kind of configuration is part of the application and shouldn’t be altered by an implementation detail such as the infrastructure the application is running on. Problem is that people go looking in the docs for logging configuration and there’s no mentioning of this to be found, making it all the more confusing. |
The issue with this change is that it's overriding the behavior set in appsettings.json and causing a lot of confusion. For example we have a boilerplate based on this and we've spent a significant amount of time investigating why the logs are suddenly formatter as JSON although we have configuration set to Simple Console. This change is not cancelable in the image causing the setting to be ignored and thus more confusion for the developers. Anyone can set the configuration on appsettings.json or in the image, but unset it is quite complex. |
|
I feel like we should take the breaking change hit, but I'm not sure if we have prior art here. I think we revert the change and make an announcement. |
|
Who is the active PM for diagnostics? They should be chiming in. |
The PM was @shirhatti, but he recently left Microsoft so I think the decision will fall to us. He did discuss it with me in the past and the rationale I recall is much the same as what he wrote above. I think it just came down to a belief (perhaps an inaccurate one given feedback) that people would value JSON input for its integration with automated log collectors more than they would miss the original format for human readability. We can replicate the analysis on how the logs appear under different human and automated log collection scenarios but I expect everyone will agree which scenarios look better and which look worse, and then it will still be a judgement call on picking which scenarios get the benefit of the default and which do not. In terms of options to move forward various thoughts:
I assume we should do (4) no matter what. None of the options are that satisfying and a lot of it comes down to what precedent/policy we've created for change in a servicing release on ASP.Net which I am not in the best position to judge. For example historically on .NET Framework I don't expect we would have ever taken a servicing behavior change this large unless it was for a security issue or huge negative feedback (far more than what we've seen so far). However I also know ASP.Net has been pioneering more lenient breaking change policy so I try not to lean on my past .NET Framework intuitions too much. We could at least reduce this from big confusion to small annoyance with better notification + easy opt-out. The more I think on it maybe that is the right balance for now, followed by removal in .NET 7 if feedback continues to be negative. |
|
@noahfalk - Thanks for the writeup. From lessons learned maintaining our .NET Docker offering, I am hesitant of #1. We have made breaking changes in the past to our Dockerfiles and it typically doesn't pan out well. While 6.0 is still in its lifecycle infancy, we have a lot of early adoption. I believe better notification + easy opt-out is the right choice here w/the option to revert the behavior in 7.0. Do you have thoughts on what the following would look like?
|
|
I like 1, then 5, then 2 |
I was hand-waving solutions where the developer might add something like this in their app's docker file: This solution wouldn't work right now, but presumably a future servicing change could add code somewhere in .NET so that the setting got recognized as being an opt-out. |
|
I appreciate doing development in the open, and discussing language features in github PRs and Issues. I Really like how connected with the community C# is, so props! The way this change was handled is definitely not cool. It's important to realise that the ecosystem is used by developers as small building blocks to build large and complex solutions. When making breaking changes, you should always strive for simplicity of removing differences rather than adding more rules for specific cases. Like everyone else we run dotnet in k8s in production, in containers on developers' machines, and out of containers. Whatever custom rules you create will not capture all the usecases. Logging configs / appsettings overrides are already complicated, and adding more custom rules that are very specific should not be attractive. |
/cc @ldillonel |
|
updated comment @YarekTyshchenko sorry to hear that this has caused churn on your end. For those just arriving to this issue, I am aiming to summarize the conversation here: SummarySo in this issue we introduced this breaking change behavior so that in a container the default console logger would be json, in other words it sets environment variable Possible workaround:Setting this in the Dockerfile should work: Todo nextGiven the incoming feedback we received there's a selection of next action options to choose from listed in #2725 (comment)
My take would be to go for option number 5, also since we're already past the adoption period. This way if someone wanted this reverted I guess they also have the option to set FormatterName in the appsettings.json file |
|
Thankyou, @maryamariyan. We've using the workaround because we build our own images in this case.
Definitely do not change the precedence between appsettings and env vars in general. We rely on env vars overriding appsettings files, I'm sure many do. IMO its good to keep human readable format as the default logger, as when people move to JSON logging they can either update their appsettings, or add the formatter env var to their image builds, or deployed env. My preference is for opt in, because JSON logs are completely unreadable to me without extra tooling. Alternatively you could play with the indenting in the JSON formatter to output multiline (possibly even highlighted, if dotnet binary is outputting to a real terminal) format that is easy to read, yet still parsable by JSON logging ingestion. I think then people wouldn't mind that being the default. Of course the change would have to be done in dotnet, not just the image to make it overridable in appsettinsg and environment, as expected. |
|
I definitely agree about the opt-in, not opt-out. The way my application behaves should not magically change just because I choose to host it in a container. Console logging is functionality and observable output. It is not an implementation detail. My choice to host my app in a container should have absolutely nothing to do with the format of the console logs. Also, nothing else I know of that runs in containers defaults to JSON output in console. I use k8s a lot, and looking at any of the pod logs across any of my clusters (system pods or 3rd party pods) - none of them are JSON format. .NET is now suddenly completely inconsistent with everything else. |
This is conflating things. The fact that some ENVs comes with the image is irrelevant from a precedence standpoint. What isn't irrelevant is the fact that the ENV was added. That shouldn't have been done. The order should be:
That's how version-selection and roll-forward work, for example. |
|
Hey folks, an update to revert this change is on the way in May: #3642. |
|
Who can explain, why logging configuration is unexpectedly getting from appsettings.json at the same time as other configuration stuff is getting from secrets.json as it should be in Development environment? How is it being managed? |
Currently, the console logger in
CreateDefaultBuilder()defaults to a multiline human-readable output. This doesn’t work well in container environments where folks use tools like fluentd or Azure Monitor to collect those logs.https://github.com/dotnet/runtime/blob/8a52f1e948b6f22f418817ec1068f07b8dae2aa5/src/libraries/Microsoft.Extensions.Hosting/src/Host.cs#L108
Some limitations of the current formatter:
What I would like to do is to switch the default
ConsoleLoggerFormatterto output structured JSON logs instead of our current format when running in a container.Old output:
New output:
Proposal
Use the
DOTNET_RUNNING_IN_CONTAINERenvironment variable to control whichConsoleFormatterwe default to.DOTNET_RUNNING_IN_CONTAINERQuestions
Unless folks opt-in to using ILogger there’s no way to make this change to all images. We can do this for all hosted (ASP.NET/worker) app models.
If you chose to Console.WriteLine directly, there’s not much we can (or should) do.
Possibly? That’s why I was hoping to solicit feedback before making the change.
Looking at the common deployment targets for hosted app models:
Of the top four most common deployment targets, it seems like Linux containers are the only scenario where defaulting to json makes sense.
However, given that the change is mostly inconsequential on Windows, I could be convinced that this change needs to made more broadly.
There’s no impact here to other processes in a container. The change we’re discussing only impacts the output of hosted applications. If you do happen to build and run in an SDK container, then yes I expect the console logging from your hosted application to be JSON formatted.
Yes, I propose we gate this change solely on running in a container. The hosting environment (
IWebHostEnvironment) has no impact.👀 @maryamariyan @richlander @davidfowl @noahfalk
The text was updated successfully, but these errors were encountered: