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

Upstream MS changes #382

Merged
merged 4 commits into from
Oct 21, 2021
Merged

Upstream MS changes #382

merged 4 commits into from
Oct 21, 2021

Conversation

aslicerh
Copy link
Member

These are changes to bring us more in line with some of MS's default setup.
See the following:
https://github.com/dotnet/dotnet-docker/pull/2710/files
dotnet/dotnet-docker#2725

@tmds
Copy link
Member

tmds commented Oct 15, 2021

Set default logging style in runtime images, unset it in build images.

Because the way s2i works, most applications run in the build images. This change won't have the effect of causing most application to run with the json output.

I think many of our users end up looking at the text in the OpenShift console.
Having the json output there would be harder to grok imo.

I prefer users set this envvar themselves based on the log collection infra they use in OpenShift.

Set DOTNET_NOLOGO=true in the runtime.

I'm ok with replacing dotnet help with this.

Should this be in the build image? I'd guess it gets read by the SDK.

note: because we're building a .NET app in the build image (the container-tool), we actually need neither.

@omajid
Copy link
Member

omajid commented Oct 19, 2021

I think many of our users end up looking at the text in the OpenShift console.
Having the json output there would be harder to grok imo.

Maybe. I think it's worth keeping format in json, for several reasons.

  1. Consistency with Microsoft's container images

    Microsoft is making this change in their .NET 6 containers. Not doing this in our images is diverging from what developers using Microsoft containers will expect. That's obviously okay, if there are good reasons. But anyone who follows any tutorials originally written against Microsoft's container images and runs them against our container images will be surprised and without a solution.

  2. OpenShift can not handle multi-line logging by default in any sane way

    We expect most of our users are enterprise developers who care about support, security and maintenance. The same set of audience is likely to be using centralized logging/monitoring. OpenShift uses fluentd for logging, by default: https://docs.openshift.com/container-platform/4.9/logging/cluster-logging.html#cluster-logging-about-components_cluster-logging. This default text-based format breaks that since fluentd can not track multi-line logs. It's literally not supported: https://access.redhat.com/solutions/5507871.. There are some RFEs, such as https://issues.redhat.com/browse/LOG-843, in progress. The likely implementation, https://github.com/GoogleCloudPlatform/fluent-plugin-detect-exceptions/blob/master/lib/fluent/plugin/exception_detector.rb, doesn't support .NET.

    In other words, for our target audience - enterprises - we should log in a format that's consistent with our official support/recommendations.

Edit: alternatively, maybe we should consider adding .NET support to fluent-plugin-detect-exceptions?

I prefer users set this envvar themselves based on the log collection infra they use in OpenShift.

If that's the route we want to go down, we should document this, so they know they can easily adjust the log format, as well as how to do it.

@tmds
Copy link
Member

tmds commented Oct 20, 2021

Maybe. I think it's worth keeping format in json, for several reasons.

If we have it, we need to set it in both the runtime and sdk image.

Consistency with Microsoft's container images

I think Microsoft's decision is driven by how things work/look best in Azure. We need to look from the OpenShift perspective.

OpenShift can not handle multi-line logging by default in any sane way

In the pod log it's not an issue afaik.

Because the pod log UI doesn't do any formatting, I think that, for humans, the text format is preferable because it is easier to understand, and matches what is shown during development.

OpenShift uses fluentd for logging, by default:

I've known this, but I have not seen it exposed.

It's not clear how making the format more fluend friendly, makes the user experience better.

If that's the route we want to go down, we should document this, so they know they can easily adjust the log format, as well as how to do it.

It should be documented as part of something that describes how to best integrate .NET logging into OpenShift log infra.

My personal experience is limited to looking at the pods logs.

@tmds
Copy link
Member

tmds commented Oct 21, 2021

It's not clear how making the format more fluend friendly, makes the user experience better.

@omajid @aslicerh we can change the logging when we know the answer to this.

I'm going to merge this PR as it is now. We can make additional PRs for the logging format.

@tmds tmds merged commit 40e13ab into redhat-developer:dev_60 Oct 21, 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