ILogger integration - part 2 by reyang · Pull Request #1315 · open-telemetry/opentelemetry-dotnet

reyang · 2020-09-29T22:56:28Z

This PR follows #1308.

Changes

Added LogProcessor interface and plumbing.
Added LogRecord struct.
Updated the getting-started demo to show how it works (The demo is now becoming complicated, will do the cleanup in another PR).

Here goes the output from ./docs/logs/getting-started $ dotnet run:

2020-09-29T22:51:22.4971101Z Program(Information, Id=0): Hello, World!
2020-09-29T22:51:22.5049270Z Program(Information, Id=0): Hello from potato 0.99.
2020-09-29T22:51:22.5069823Z Program(Information, Id=0):
    name: tomato
    price: 2.99
    $format: Hello from {name} {price}.
2020-09-29T22:51:22.5098499Z Program(Information, Id=LogEx):
    obj: Program+Food
    $format: LogEx({obj}).
2020-09-29T22:51:22.5109950Z Program(Information, Id=LogEx):
    obj: { Name = pumpkin, Price = 5.99 }
    $format: LogEx({obj}).
2020-09-29T22:51:22.5115810Z Program(Information, Id=LogEx):
    obj: System.Collections.Generic.Dictionary`2[System.String,System.Object]
    $format: LogEx({obj}).
MyProcessor.OnShutdown(5000)
MyProcessor.Dispose(True)

For significant contributions please make sure you have completed the following items:

CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

codecov · 2020-09-29T23:38:39Z

Codecov Report

Merging #1315 into master will decrease coverage by 1.44%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1315      +/-   ##
==========================================
- Coverage   78.62%   77.17%   -1.45%     
==========================================
  Files         219      223       +4     
  Lines        6265     6384     +119     
==========================================
+ Hits         4926     4927       +1     
- Misses       1339     1457     +118

Impacted Files	Coverage Δ
src/OpenTelemetry/Logs/CompositeLogProcessor.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry/Logs/LogProcessor.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry/Logs/LogRecord.cs	`0.00% <0.00%> (ø)`
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs	`0.00% <0.00%> (ø)`
...c/OpenTelemetry/Logs/OpenTelemetryLoggerOptions.cs	`0.00% <0.00%> (ø)`
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs	`0.00% <0.00%> (ø)`
... and 1 more

cijothomas · 2020-10-01T06:18:42Z

    public static void Main()
    {
+        /*
+        The OpenTelemetry style:


Though this style puts logging more aligned with Otel Tracing setup, this distances us from general ILogger guidelines.
Some thoughts:
If we go with this route, then users will have to rewire their entire logging plumbing, if they ever chose to move away from OpenTelemetry. If we stick to the existing ILogger style, then adding/removing OpenTelemetry is adding/removing one line - builder.AddOtel..(). The way user obtain ilogger instances (from logger factory or DI) does not have to change at all.

@alanwest @CodeBlanch please share your thoughts on the approaches, when you get a chance!

@cijothomas Good callout! We definitely want to fit into the standard .net core/extensions patterns. I think the code is actually fine. The only weird thing is the build-up pattern in this example project. I took a stab at fitting it into a more standard approach:

reyang/ilogger...CodeBlanch:reyang/ilogger

@CodeBlanch Thanks. The code from you branch is fitting the pattern where there is a host and/or DI . We want to provide pure console app example. Its not possible in .netcore2.1 as only way to setup ilogger requires DI, but for other versions, its possible without Hosting/DI.

I suppose then I don't understand your issue with the design 😄 LoggerProvider is registered with the Factory. The Factory creates ILogger using all registered providers. What do you see that is deviating from the ILogger guidelines?

Regarding the pattern in the sample, makes sense. BUT I think it might be more confusing than useful to people. The official guide uses the host. So our example should too IMO. If we want to provide another example, that shows how to do it without the host, OK with that, but it feels like the exception, not the rule.

@CodeBlanch There is non-host example as well now: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/logging/?view=aspnetcore-5.0#logging-providers-1

@reyang Agree. we can iterate on this and reach the right balance.

@cijothomas Gotcha. Agree with you we want to promote creating a logger through the LoggerFactory. Maybe we shouldn't even provide Sdk.CreateLoggerProviderBuilder and only have the ILoggingBuilder pattern?

@reyang Sounds good to me.

Sorry, coming to this conversation late... I agree that loggerProvider.CreateLogger is not ideal and it's best to conform to using the LoggerFactory.CreateLogger() in the context of this console application.

I think I agree with @CodeBlanch that Sdk.CreateLoggerProviderBuilder may not be required. If I'm following everyone's comments I think we'd want an extension method off of ILoggingBuilder like:

public static ILoggingBuilder AddOpenTelemetry(this ILoggingBuilder builder, Action<OpenTelemetryLoggerOptions> configure) { ... }

Posting in case you haven't read this. I find Stephen Cleary's explanation of ILogger and friends more consumable than some of the Microsoft documentation 😄. From his blog:

ILoggerFactory is a collection of ILoggerProviders that creates composite ILogger/ILogger loggers.
ILoggerProvider ia a provider for a specific logging system. It provides ILogger loggers to the ILoggerFactory.

That is, it is expected that someone configure all of their logging related concerns through the ILoggingBuilder and then use ILoggerFactory to get ILogger instances. It would be very unusual for someone to use an ILoggerProvider directly.

Also agree that we can iterate on this.

I've removed Sdk.CreateLoggerProviderBuilder based on the feedback here.

ThomsonTan · 2020-10-02T21:59:08Z

+#endif
        {
-            builder.AddOpenTelemetry();
+            builder.AddOpenTelemetry(options => options.AddProcessor(new MyProcessor()));


Duplicate builder.AddOpenTelemetry() to the above #if/#else/#endif, so we don't need another #if/#else/#endif below might make the code more friendly to new comer?

As I've explained in #1315 (comment), have a simple tutorial/demo is not in this PR's scope.

cijothomas · 2020-10-05T15:56:06Z

+        /// Adds processor to the provider.
+        /// </summary>
+        /// <param name="processor">Log processor to add.</param>
+        /// <returns>Returns <see cref="LoggerProviderBuilder"/> for chaining.</returns>


comment is outdated now. This is returning OpenTelemetryLoggerOptions

cijothomas · 2020-10-05T15:59:34Z

+    /// <summary>
+    /// Build OpenTelemetryLoggerProvider with Processors.
+    /// </summary>
+    public class LoggerProviderBuilder


We can remove this class now?

cijothomas

LGTM - left couple of small comments.

add log processor

a2017af

reyang requested a review from a team September 29, 2020 22:56

fix changelog

ed01a3e

reyang commented Sep 29, 2020

View reviewed changes

Comment thread src/OpenTelemetry.Api/CHANGELOG.md

reyang commented Sep 29, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/CompositeLogProcessor.cs

macros

decd4b7

cijothomas reviewed Sep 29, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/OpenTelemetryLogger.cs Outdated

cijothomas reviewed Sep 29, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/OpenTelemetryLogger.cs

more conditional compilations

e4dd3ce

CodeBlanch reviewed Sep 30, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/LogRecord.cs Outdated

CodeBlanch reviewed Sep 30, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/LogRecord.cs Outdated

ThomsonTan reviewed Sep 30, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/LogProcessor.cs

ThomsonTan reviewed Sep 30, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/LogProcessor.cs

reyang and others added 2 commits September 29, 2020 17:51

improve struct

8176931

Merge branch 'master' into reyang/ilogger

d271d10

reyang mentioned this pull request Sep 30, 2020

Consistent ArgumentOutOfRangeException message #1316

Closed

remove duplicated stuff

d6ccc71

cijothomas reviewed Oct 1, 2020

View reviewed changes

Merge branch 'master' into reyang/ilogger

167ed98

ThomsonTan reviewed Oct 2, 2020

View reviewed changes

alanwest reviewed Oct 3, 2020

View reviewed changes

Comment thread src/OpenTelemetry/Logs/OpenTelemetryLoggerOptions.cs Outdated

remove Sdk.CreateLoggerProviderBuilder

8118e57

cijothomas reviewed Oct 5, 2020

View reviewed changes

cijothomas approved these changes Oct 5, 2020

View reviewed changes

reyang added 3 commits October 5, 2020 09:08

clean up

70b3429

Merge branch 'master' into reyang/ilogger

51816f1

update changelog

df1a3ee

cijothomas merged commit d1a3f53 into master Oct 5, 2020

cijothomas deleted the reyang/ilogger branch October 5, 2020 17:13

This was referenced Oct 6, 2020

Consolidate log and trace processor #1322

Merged

Add a value type version of CircularBuffer #1325

Merged

Add a value type version of Batch<T> #1327

Closed

Conversation

reyang commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Sep 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alanwest Oct 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cijothomas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

reyang commented Sep 29, 2020 •

edited

Loading

codecov Bot commented Sep 29, 2020 •

edited

Loading

alanwest Oct 3, 2020 •

edited

Loading