Skip to content

Adds Console Log Formatting APIs - #38616

Merged
maryamariyan merged 56 commits into
dotnet:masterfrom
maryamariyan:pr-feedback
Jul 15, 2020
Merged

Adds Console Log Formatting APIs#38616
maryamariyan merged 56 commits into
dotnet:masterfrom
maryamariyan:pr-feedback

Conversation

@maryamariyan

@maryamariyanmaryamariyan commented Jun 30, 2020

Copy link
Copy Markdown
Contributor
  • APIs approved from issue: Flexible and efficient optionally-structured console logging out of the box #34742
  • Apply API review feedback
  • Add implementation for the APIs.
  • Updated ConsoleLoggerTests -> contains existing test cases (supporting deprecated workflow and using obsolete APIs to ensure 3.1 workflow is not regressed.)
  • minor styling bugfixes (e.g. exception log did not have proper padding on Default format)
  • The Log workflow would handle VT100 parsing for basic cases only. The parser ignores the escape codes we don't support. When a full parser is implemented later in the BCL, we'll switch the logger code to use that one instead
  • add new test cases

@maryamariyanmaryamariyan added NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) area-Extensions-Logging labels Jun 30, 2020
@maryamariyanmaryamariyan self-assigned this Jun 30, 2020
@Dotnet-GitSync-Bot

Copy link
Copy Markdown
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@tarekgh

Copy link
Copy Markdown
Member

@maryamariyan I'll wait till the design review occur and then will review this PR.

CC @davidfowl

public const string Json = "json";
public const string Systemd = "systemd";
}
[System.ObsoleteAttribute("ConsoleLoggerFormat has been deprecated.", false)]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ObsoleteAttribute [](start = 12, length = 17)

Do we need to file and fill the breaking change template for such changes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say we don't. @PriyaPurkayastha?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We actually do document APIs marked obsolete as a breaking change using https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md.
You can look up https://docs.microsoft.com/en-us/dotnet/core/compatibility/aspnetcore for examples.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • TODO

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLogger.cs Outdated
@ericstjericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Jul 1, 2020
Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLogger.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/StringWriter.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/StringWriter.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/StringWriter.cs Outdated
Comment threadsrc/libraries/Microsoft.Extensions.Logging.Console/src/StringWriter.cs Outdated
@jkotas

Copy link
Copy Markdown
Member

Was CI green on this before this was merged? I am seeing failures introduced by this change in number of PRs.

@maryamariyan

Copy link
Copy Markdown
ContributorAuthor

yes. it was green and I did a rebase to resolve a conflict on ConsoleLoggerTest

@maryamariyan

maryamariyan commented Jul 16, 2020

Copy link
Copy Markdown
ContributorAuthor

this git page is incorrectly saying 168 files were changed, but it is actually 47 files: see 69d98ee

@safern

Copy link
Copy Markdown
Member

Was CI green on this before this was merged? I am seeing failures introduced by this change in number of PRs.

I'm investigating the failure and I believe this is because of: #39209

kernel32.dll shouldn't come into play in OSX nor Linux and Linux and OSX test runs are failing with:

Microsoft.Extensions.Hosting.HostTests.CreateDefaultBuilder_IncludesContentRootByDefault [FAIL]
System.DllNotFoundException : Unable to load shared library 'kernel32.dll' or one of its dependencies. In order to help diagnose loading problems, consider setting the DYLD_PRINT_LIBRARIES environment variable: dlopen(libkernel32.dll, 1): image not found
Stack Trace:
at Microsoft.Extensions.Logging.Console.ConsoleLoggerProvider.DoesConsoleSupportAnsi()
/_/src/libraries/Microsoft.Extensions.Logging.Console/src/ConsoleLoggerProvider.cs(50,0): at Microsoft.Extensions.Logging.Console.ConsoleLoggerProvider..ctor(IOptionsMonitor`1 options, IEnumerable`1 formatters)

I'm doing a run without #39209

@ghostghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Extensions-Loggingbreaking-changeIssue or PR that represents a breaking API or functional change over a previous release.new-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@maryamariyan@Dotnet-GitSync-Bot@tarekgh@adamsitnik@safern@eerhardt@jkotas@davidfowl@tmds@terrajobst@ericstj@layomia@joperezr@PriyaPurkayastha@monojenkins@GrabYourPitchforks@hoyosjs@akoeplinger@davidwrighton@janvorli