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

The working directory log message on app startup #85809

Closed
JamesNK opened this issue May 5, 2023 · 5 comments · Fixed by #86057
Closed

The working directory log message on app startup #85809

JamesNK opened this issue May 5, 2023 · 5 comments · Fixed by #86057
Assignees
Milestone

Comments

@JamesNK
Copy link
Member

JamesNK commented May 5, 2023

Description

#82445 added a new log message when an app starts (last line):

info: Microsoft.Hosting.Lifetime[14]
      Now listening on: http://localhost:5000
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\Development\source\Benchmarks\src\BenchmarksApps\Grpc\BasicGrpc
info: Microsoft.Hosting.Lifetime[0]
      Current working directory is /. If Content root path is not set explicitly, then working directory is used by default.

This is a very prominent place for logging. Every ASP.NET Core app displays this log message by default when it starts. Exact thought needs to be put into what is logged here and it seems like it needs more work.

  • Weird casing. Why is "Content" capitalized in "If Content root ..."? It looks like a typo.
  • The log message is inconsistent with the other logging written during startup
  • Also, the meaning of the working directory value in logging is unclear. What does / mean? Should this be a full path?
  • Is information the right log level? Should this be displayed by default? The default ASP.NET Core log level is Information. If this is only useful for debugging then perhaps the Debug log level would be better. That reduces logs written on app start.
  • Needs unit tests

Reproduction Steps

Start an app with hosting, e.g. an aspnetcore app, with console logging. The log is written to the console:

Expected behavior

Log message is consistent with other logs on startup. Working directory value is easy to understand.

Actual behavior

info: Microsoft.Hosting.Lifetime[14]
      Now listening on: http://localhost:5000
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\Development\source\Benchmarks\src\BenchmarksApps\Grpc\BasicGrpc
info: Microsoft.Hosting.Lifetime[0]
      Current working directory is /. If Content root path is not set explicitly, then working directory is used by default.

Hosting environment and content root path logs have the format Name: value. I would expect the current working directory to be the same, e.g. Working directory: XXX.

Also, this log seems the wrong place to add extra detail like If Content root path is not set explicitly, then working directory is used by default.. This logging is visible by default in every ASP.NET Core app that starts up. Only the most important detail should be logged here. I don't think that sentence meets the bar.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@ghost ghost added the untriaged New issue has not been triaged by the area owner label May 5, 2023
@ghost
Copy link

ghost commented May 5, 2023

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

#82445 added a new log message when an app starts. This is a very prominent place for logging (i.e. every ASP.NET Core app displays it by default when it starts) and it seems like it needs more work.

  • Weird casing. Why is "Content" capitalized in "If Content root ..."? It looks like a typo.
  • The log message is inconsistent with the other logging written during startup
  • Also the meaning of the working directory value in logging is unclear
  • Lacks unit tests

Reproduction Steps

Start an app with hosting, e.g. an aspnetcore app, with console logging. The log is written to the console:

Expected behavior

Log message is consistent with other logs on startup. Working directory is easy to understand.

Actual behavior

info: Microsoft.Hosting.Lifetime[14]
      Now listening on: http://localhost:5000
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Development
info: Microsoft.Hosting.Lifetime[0]
      Content root path: C:\Development\source\Benchmarks\src\BenchmarksApps\Grpc\BasicGrpc
info: Microsoft.Hosting.Lifetime[0]
      Current working directory is /. If Content root path is not set explicitly, then working directory is used by default.

Hosting environment and content root path logs have the format Name: value. I would expect the current working directory to be the same, e.g. Working directory: XXX.

Also, this log seems the wrong place to add extra detail like "If Content root path is not set explicitly, then working directory is used by default.". It's visible by default in every ASP.NET Core app that starts up. Only the most important detail should be logged here.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: JamesNK
Assignees: -
Labels:

area-Extensions-Hosting

Milestone: -

@JamesNK
Copy link
Member Author

JamesNK commented May 5, 2023

cc @DamianEdwards @davidfowl

@davidfowl
Copy link
Member

I think we should revert this. It's unclear why this is useful information to show all of the time.

cc @steveharter @ericstj

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label May 10, 2023
@ericstj ericstj added this to the 8.0.0 milestone May 10, 2023
@ericstj
Copy link
Member

ericstj commented May 10, 2023

Let's have another look at this, @steveharter. CC @pedrobsaila

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 10, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 10, 2023
@steveharter
Copy link
Member

The commit was reverted and the original issue was reopened.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants