-
Notifications
You must be signed in to change notification settings - Fork 784
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
ILogger integration - part 2 #1315
Changes from 8 commits
a2017af
ed01a3e
decd4b7
e4dd3ce
8176931
d271d10
d6ccc71
167ed98
8118e57
70b3429
51816f1
df1a3ee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,85 @@ | ||
// <copyright file="MyProcessor.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using OpenTelemetry; | ||
using OpenTelemetry.Logs; | ||
|
||
internal class MyProcessor : LogProcessor | ||
{ | ||
private readonly string name; | ||
|
||
public MyProcessor(string name = "MyProcessor") | ||
{ | ||
this.name = name; | ||
} | ||
|
||
public override void OnLog(in LogRecord record) | ||
{ | ||
var state = record.State; | ||
|
||
if (state is IReadOnlyCollection<KeyValuePair<string, object>> dict) | ||
{ | ||
var isUnstructuredLog = dict.Count == 1; | ||
|
||
if (isUnstructuredLog) | ||
{ | ||
foreach (var entry in dict) | ||
{ | ||
Console.WriteLine($"{record.Timestamp:yyyy-MM-ddTHH:mm:ss.fffffffZ} {record.CategoryName}({record.LogLevel}, Id={record.EventId}): {entry.Value}"); | ||
} | ||
} | ||
else | ||
{ | ||
Console.WriteLine($"{record.Timestamp:yyyy-MM-ddTHH:mm:ss.fffffffZ} {record.CategoryName}({record.LogLevel}, Id={record.EventId}):"); | ||
foreach (var entry in dict) | ||
{ | ||
if (string.Equals(entry.Key, "{OriginalFormat}", StringComparison.Ordinal)) | ||
{ | ||
Console.WriteLine($" $format: {entry.Value}"); | ||
continue; | ||
} | ||
|
||
Console.WriteLine($" {entry.Key}: {entry.Value}"); | ||
} | ||
} | ||
|
||
if (record.Exception != null) | ||
{ | ||
Console.WriteLine($" $exception: {record.Exception}"); | ||
} | ||
} | ||
} | ||
|
||
protected override bool OnForceFlush(int timeoutMilliseconds) | ||
{ | ||
Console.WriteLine($"{this.name}.OnForceFlush({timeoutMilliseconds})"); | ||
return true; | ||
} | ||
|
||
protected override bool OnShutdown(int timeoutMilliseconds) | ||
{ | ||
Console.WriteLine($"{this.name}.OnShutdown({timeoutMilliseconds})"); | ||
return true; | ||
} | ||
|
||
protected override void Dispose(bool disposing) | ||
{ | ||
Console.WriteLine($"{this.name}.Dispose({disposing})"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,17 +15,40 @@ | |
// </copyright> | ||
|
||
using System.Collections.Generic; | ||
#if NETCOREAPP2_1 | ||
using Microsoft.Extensions.DependencyInjection; | ||
#endif | ||
using Microsoft.Extensions.Logging; | ||
using OpenTelemetry; | ||
|
||
public class Program | ||
{ | ||
public static void Main() | ||
{ | ||
/* | ||
The OpenTelemetry style: | ||
|
||
using var loggerProvider = Sdk.CreateLoggerProviderBuilder() | ||
.AddProcessor(new MyProcessor()) | ||
.Build(); | ||
var logger = loggerProvider.CreateLogger("MyLogger"); | ||
*/ | ||
|
||
#if NETCOREAPP2_1 | ||
var serviceCollection = new ServiceCollection().AddLogging(builder => | ||
#else | ||
using var loggerFactory = LoggerFactory.Create(builder => | ||
#endif | ||
{ | ||
builder.AddOpenTelemetry(); | ||
builder.AddOpenTelemetry(options => options.AddProcessor(new MyProcessor())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Duplicate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I've explained in #1315 (comment), have a simple tutorial/demo is not in this PR's scope. |
||
}); | ||
|
||
#if NETCOREAPP2_1 | ||
using var serviceProvider = serviceCollection.BuildServiceProvider(); | ||
var logger = serviceProvider.GetRequiredService<ILogger<Program>>(); | ||
#else | ||
var logger = loggerFactory.CreateLogger<Program>(); | ||
#endif | ||
|
||
// unstructured log | ||
logger.LogInformation("Hello, World!"); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,18 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<TargetFramework>netcoreapp3.1</TargetFramework> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" /> | ||
<ProjectReference Include="$(RepoRoot)\src\OpenTelemetry\OpenTelemetry.csproj" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' == 'netcoreapp2.1'"> | ||
<PackageReference Include="Microsoft.Extensions.Logging" Version="2.1.1" /> | ||
<PackageReference Include="Microsoft.Extensions.DependencyInjection" Version="2.1.1" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup Condition="'$(TargetFramework)' != 'netcoreapp2.1'"> | ||
<PackageReference Include="Microsoft.Extensions.Logging" Version="$(MicrosoftExtensionsLoggingPkgVer)" /> | ||
</ItemGroup> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
// <copyright file="CompositeLogProcessor.cs" company="OpenTelemetry Authors"> | ||
// Copyright The OpenTelemetry Authors | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
// </copyright> | ||
|
||
#if NETSTANDARD2_0 | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.Threading; | ||
using OpenTelemetry.Internal; | ||
|
||
namespace OpenTelemetry.Logs | ||
{ | ||
public class CompositeLogProcessor : LogProcessor | ||
reyang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
private DoublyLinkedListNode<LogProcessor> head; | ||
private DoublyLinkedListNode<LogProcessor> tail; | ||
private bool disposed; | ||
|
||
public CompositeLogProcessor(IEnumerable<LogProcessor> processors) | ||
{ | ||
if (processors == null) | ||
{ | ||
throw new ArgumentNullException(nameof(processors)); | ||
} | ||
|
||
using var iter = processors.GetEnumerator(); | ||
|
||
if (!iter.MoveNext()) | ||
{ | ||
throw new ArgumentException($"{nameof(processors)} collection is empty"); | ||
} | ||
|
||
this.head = new DoublyLinkedListNode<LogProcessor>(iter.Current); | ||
this.tail = this.head; | ||
|
||
while (iter.MoveNext()) | ||
{ | ||
this.AddProcessor(iter.Current); | ||
} | ||
} | ||
|
||
public CompositeLogProcessor AddProcessor(LogProcessor processor) | ||
{ | ||
if (processor == null) | ||
{ | ||
throw new ArgumentNullException(nameof(processor)); | ||
} | ||
|
||
var node = new DoublyLinkedListNode<LogProcessor>(processor) | ||
{ | ||
Previous = this.tail, | ||
}; | ||
this.tail.Next = node; | ||
this.tail = node; | ||
|
||
return this; | ||
} | ||
|
||
/// <inheritdoc/> | ||
public override void OnLog(in LogRecord record) | ||
{ | ||
var cur = this.head; | ||
|
||
while (cur != null) | ||
{ | ||
cur.Value.OnLog(record); | ||
cur = cur.Next; | ||
} | ||
} | ||
|
||
/// <inheritdoc/> | ||
protected override bool OnForceFlush(int timeoutMilliseconds) | ||
{ | ||
var cur = this.head; | ||
|
||
var sw = Stopwatch.StartNew(); | ||
|
||
while (cur != null) | ||
{ | ||
if (timeoutMilliseconds == Timeout.Infinite) | ||
{ | ||
_ = cur.Value.ForceFlush(Timeout.Infinite); | ||
} | ||
else | ||
{ | ||
var timeout = (long)timeoutMilliseconds - sw.ElapsedMilliseconds; | ||
|
||
if (timeout <= 0) | ||
{ | ||
return false; | ||
} | ||
|
||
var succeeded = cur.Value.ForceFlush((int)timeout); | ||
|
||
if (!succeeded) | ||
{ | ||
return false; | ||
} | ||
} | ||
|
||
cur = cur.Next; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
/// <inheritdoc/> | ||
protected override bool OnShutdown(int timeoutMilliseconds) | ||
{ | ||
var cur = this.head; | ||
var result = true; | ||
var sw = Stopwatch.StartNew(); | ||
|
||
while (cur != null) | ||
{ | ||
if (timeoutMilliseconds == Timeout.Infinite) | ||
{ | ||
result = cur.Value.Shutdown(Timeout.Infinite) && result; | ||
} | ||
else | ||
{ | ||
var timeout = (long)timeoutMilliseconds - sw.ElapsedMilliseconds; | ||
|
||
// notify all the processors, even if we run overtime | ||
result = cur.Value.Shutdown((int)Math.Max(timeout, 0)) && result; | ||
} | ||
|
||
cur = cur.Next; | ||
} | ||
|
||
return result; | ||
} | ||
|
||
protected override void Dispose(bool disposing) | ||
{ | ||
if (this.disposed) | ||
{ | ||
return; | ||
} | ||
|
||
if (disposing) | ||
{ | ||
var cur = this.head; | ||
|
||
while (cur != null) | ||
{ | ||
try | ||
{ | ||
cur.Value?.Dispose(); | ||
} | ||
catch (Exception ex) | ||
{ | ||
OpenTelemetrySdkEventSource.Log.SpanProcessorException(nameof(this.Dispose), ex); | ||
} | ||
|
||
cur = cur.Next; | ||
} | ||
} | ||
|
||
this.disposed = true; | ||
} | ||
|
||
private class DoublyLinkedListNode<T> | ||
{ | ||
public readonly T Value; | ||
|
||
public DoublyLinkedListNode(T value) | ||
{ | ||
this.Value = value; | ||
} | ||
|
||
public DoublyLinkedListNode<T> Previous { get; set; } | ||
|
||
public DoublyLinkedListNode<T> Next { get; set; } | ||
} | ||
} | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanwest @CodeBlanch please share your thoughts on the approaches, when you get a chance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reyang Agree. we can iterate on this and reach the right balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 theILoggingBuilder
pattern?@reyang Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, coming to this conversation late... I agree that
loggerProvider.CreateLogger
is not ideal and it's best to conform to using theLoggerFactory.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: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:That is, it is expected that someone configure all of their logging related concerns through the
ILoggingBuilder
and then useILoggerFactory
to getILogger
instances. It would be very unusual for someone to use anILoggerProvider
directly.Also agree that we can iterate on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed
Sdk.CreateLoggerProviderBuilder
based on the feedback here.