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

Align console logging example with ASP.NET Core logging example #4855

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

reyang
Copy link
Member

@reyang reyang commented Sep 15, 2023

Essentially stealing good stuff from #4821.

@reyang reyang requested a review from a team September 15, 2023 06:05
@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #4855 (7ac5189) into main (fe78453) will increase coverage by 0.04%.
Report is 2 commits behind head on main.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4855      +/-   ##
==========================================
+ Coverage   83.91%   83.95%   +0.04%     
==========================================
  Files         293      293              
  Lines       12028    12021       -7     
==========================================
- Hits        10093    10092       -1     
+ Misses       1935     1929       -6     
Files Changed Coverage
src/OpenTelemetry.Shims.OpenTracing/TracerShim.cs ø

namespace SourceGeneration;

public class Program
using var loggerFactory = LoggerFactory.Create(builder =>
Copy link
Member

Choose a reason for hiding this comment

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

not something introduced in this PR, but the "using" is accidentally/un-intentionally copied as-is by a lot of users into their helper methods, and the LoggerFactory ends up disposed when the helper method exits.

Copy link
Member Author

@reyang reyang Sep 18, 2023

Choose a reason for hiding this comment

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

What options do we have and which one do you like?

  1. Remove using, leave Dispose to GC finalizer (if there is a finalizer).
  2. Add a comment to tell the user.
  3. Remove using, add an explicit Dispose at the end of the application.

I'll give 1) -100, 2) +2, 3) +1

Copy link
Member

Choose a reason for hiding this comment

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

A combination of 2, 3 i.e remove using, and add explicit Dispose at the end. And comments saying

  1. Loggerfactory must be kept active for the logging to work. (to warn about disposing too early)
  2. Loggerfactory must be disposed at the end/shutdown to make sure any in-buffers telemetry is pushed out as well. (to warn about not disposing at all)

Not something to be fixed for this PR, as this issue is there for other getting-started docs as well

@utpilla utpilla merged commit 7eb3e73 into open-telemetry:main Sep 18, 2023
@reyang reyang deleted the reyang/console-logging-tweaks branch March 2, 2024 04:02
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.

4 participants