LogEmitter System.Diagnostics.Tracing extension project #3305

CodeBlanch · 2022-05-25T00:03:46Z

Changes

Adds extensions for System.Diagnostics.Tracing

Proposed Public API

namespace OpenTelemetry.Logs
{
    public class OpenTelemetryLoggerProvider
    {
+       public bool IncludeFormattedMessage { get; }
    }
}

TODOs

Appropriate CHANGELOG.md updated for non-trivial changes
Changes in public API reviewed

codecov · 2022-05-25T00:13:53Z

Codecov Report

Merging #3305 (ffda1ce) into main (ab10e11) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head ffda1ce differs from pull request most recent head cf69b8c. Consider uploading reports for the commit cf69b8c to get more accurate results

@@            Coverage Diff             @@
##             main    #3305      +/-   ##
==========================================
- Coverage   86.21%   86.17%   -0.04%     
==========================================
  Files         265      263       -2     
  Lines        9598     9543      -55     
==========================================
- Hits         8275     8224      -51     
+ Misses       1323     1319       -4

Impacted Files	Coverage Δ
.../OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs	`96.10% <100.00%> (+0.05%)`	⬆️
src/OpenTelemetry/Logs/Pool/LogRecordSharedPool.cs	`78.94% <0.00%> (-21.06%)`	⬇️
src/OpenTelemetry/ProviderExtensions.cs	`81.81% <0.00%> (-9.10%)`	⬇️
...Telemetry/Metrics/PeriodicExportingMetricReader.cs	`72.72% <0.00%> (-5.46%)`	⬇️
src/OpenTelemetry/Metrics/MetricReader.cs	`86.44% <0.00%> (-1.70%)`	⬇️
src/OpenTelemetry/Logs/LogRecordAttributeList.cs	`74.56% <0.00%> (-0.88%)`	⬇️
...ry/Internal/PeriodicExportingMetricReaderHelper.cs	`100.00% <0.00%> (ø)`
...qlClient/Implementation/SqlActivitySourceHelper.cs	`100.00% <0.00%> (ø)`
...tensions.Serilog/OpenTelemetrySerilogExtensions.cs
...try.Extensions.Serilog/OpenTelemetrySerilogSink.cs
... and 7 more

src/OpenTelemetry/Logs/LogRecord.cs

reyang · 2022-05-25T21:00:05Z

src/OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs

+        /// Create a <see cref="LogEmitter"/>.
+        /// </summary>
+        /// <returns><see cref="LogEmitter"/>.</returns>
+        public LogEmitter CreateEmitter() => new(this);


Do you consider LogEmitter as a pub-sub model? (similar like ActivitySource - which can be subscribed by multiple listeners)

I hadn't thought about that. There isn't much in the spec, but it seems mainly to be a way to just throw logs at a processor/exporter without using a log library. I think it would be nice/useful to do this, but should we pursue through the spec first?

CodeBlanch · 2022-05-26T20:00:08Z

This now also includes LogRecordPool. Blame @reyang for the scope creep 🤣

src/OpenTelemetry/Batch.cs

CodeBlanch · 2022-05-26T20:07:30Z

src/OpenTelemetry/Logs/LogRecordPool.cs

+    /// <summary>
+    /// Manages a pool of <see cref="LogRecord"/> instances.
+    /// </summary>
+    public sealed class LogRecordPool


I didn't spend a whole lot of time on this algorithm. Just wanted to get something workable in place that we can improve as we go. There is a fast-path using a [ThreadStatic] for simple processors executing on the same thread. For the batch case there is a shared pool which is used after the [ThreadStatic].

src/OpenTelemetry/BatchExportProcessor.cs

examples/AspNetCore/Controllers/WeatherForecastController.cs

reyang · 2022-05-27T06:56:13Z

src/OpenTelemetry/Logs/LogRecordPool.cs

+        /// Rent a <see cref="LogRecord"/> from the pool.
+        /// </summary>
+        /// <returns><see cref="LogRecord"/>.</returns>
+        public static LogRecord Rent() => current.RentCore(clearIfReused: true);


I don't have a clear position right now - would we prefer to do the clean up here or earlier (e.g. when the ref count goes to zero)? There might be small perf differences (e.g. I imagine Rent() is always called synchronously, while DecRef might be called asynchronously - from the exporter thread).

In the new iteration there isn't much to clear, log data is set fully each time (overriding what was there before). There is a bit of cleanup for the new reusable tag/attribute storage which I put on the return to the pool.

…internal.

github-actions · 2022-06-26T03:42:32Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2022-07-08T03:45:51Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

alanwest · 2022-07-08T21:09:00Z

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogExtensions.cs

+        public static LoggerConfiguration OpenTelemetry(
+            this LoggerSinkConfiguration loggerConfiguration,
+            OpenTelemetryLoggerProvider openTelemetryLoggerProvider)


For some of our other extension methods we've put the extension method in the same namespace as the class that's being extended. So, does it make sense to put this method in the Serilog.Configuration namespace, or just the root Serilog namespace? Is there an existing precedence for other Serilog extensions?

alanwest · 2022-07-08T21:19:10Z

OpenTelemetry.sln

+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Serilog", "src\OpenTelemetry.Extensions.Serilog\OpenTelemetry.Extensions.Serilog.csproj", "{F5062ED1-6B59-45FC-8E08-2F5D41A19864}"
+EndProject
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "OpenTelemetry.Extensions.Tracing", "src\OpenTelemetry.Extensions.Tracing\OpenTelemetry.Extensions.Tracing.csproj", "{F1C65913-81EC-4297-A666-A66280E3E1B6}"


Tracing is such a commonly used term. I get it comes from System.Diagnostics.Tracing, but is there a better name?

What about OpenTelemetry.Extensions.EventSource? Or if that's too narrow of a name maybe OpenTelemetry.Extensions.EventTracing? Or... ugh maybe even OpenTelemetry.Extensions.SystemDiagnosticsTracing 🥴?

alanwest · 2022-07-09T00:14:27Z

src/OpenTelemetry/Logs/OpenTelemetryLoggerProvider.cs

+        /// <summary>
+        /// Gets a value indicating whether or not formatted messages should be included on log messages.
+        /// </summary>
+        public bool IncludeFormattedMessage { get; }


Bugging you again with the "we sure we wanna expose this publicly, yet?" question 😆... I don't have a strong feeling that we shouldn't, just wanna raise it for discussion.

Brainstorming alternatives, would it make sense instead to expose this on the LogEmitter class?

At the moment, both the extensions are instantiated with an instance of the logging provider and then

store the IncludeFormattedMessage setting, then

call CreateEmitter()

Would it make sense to just instantiate the extensions with an emitter instead of the provider? So like Serilog would look like

Log.Logger = new LoggerConfiguration() .WriteTo.OpenTelemetry(openTelemetryLoggerProvider.CreateEmitter()) // <- Register OpenTelemetry Serilog sink .CreateLogger();

Again, I don't have a strong sense that one option is better than the other at this point, just curious of your thoughts.

CodeBlanch · 2022-07-18T17:39:29Z

Closing this PR because everything it was doing has been moved to smaller PRs.

LogEmitter API.

6d2f153

reyang self-requested a review May 25, 2022 01:13

reyang reviewed May 25, 2022

View reviewed changes

src/OpenTelemetry/Logs/LogRecord.cs Outdated Show resolved Hide resolved

reyang reviewed May 25, 2022

View reviewed changes

CodeBlanch added 3 commits May 25, 2022 15:01

Code review.

101ce19

Added LogRecordPool.

7eb0006

Nullable annotation updates.

20479f5

CodeBlanch changed the title ~~[SDK] LogEmitter API~~ [SDK] LogEmitter API + LogRecordPool May 26, 2022

Merge branch 'main' into log-emitter

c64ca11

CodeBlanch commented May 26, 2022

View reviewed changes

src/OpenTelemetry/Batch.cs Outdated Show resolved Hide resolved

CodeBlanch commented May 26, 2022

View reviewed changes

CodeBlanch added 2 commits May 26, 2022 13:21

Cleanup.

53a6854

Cleanup.

fd5a87e

reyang reviewed May 26, 2022

View reviewed changes

src/OpenTelemetry/BatchExportProcessor.cs Outdated Show resolved Hide resolved

Added reference counting into the log record pool.

035823b

reyang reviewed May 27, 2022

View reviewed changes

examples/AspNetCore/Controllers/WeatherForecastController.cs Outdated Show resolved Hide resolved

reyang reviewed May 27, 2022

View reviewed changes

CodeBlanch added 11 commits May 28, 2022 11:05

Tweaks.

80cbdbb

Tweak.

744c399

Test fix.

df93cf5

Merge branch 'main' into log-emitter

1ba8afe

Test fix.

9a0ee32

Rename.

888eb54

Trigger state buffering by processor inspection.

558e8d2

Implement copy for in-memory log exporter.

4d76b87

Added GetDataRef.

617d88d

Tweaks.

ca7e319

Merge branch 'main' into log-emitter

6fae76d

Fix merge.

cb69e49

CodeBlanch mentioned this pull request Jun 16, 2022

SDK: Nullable annotations for base classes & batch + shims to enable compiler features #3374

Merged

CodeBlanch added 2 commits June 17, 2022 10:55

Merge from main.

8693d93

Updates.

b71006f

CodeBlanch mentioned this pull request Jun 17, 2022

Logs: Add LogRecordData #3378

Merged

CodeBlanch added 2 commits June 17, 2022 21:43

Merge from main.

6269015

Revert OtlpLogExporter use of logRecord.GetDataRef because it is now …

7647145

…internal.

CodeBlanch mentioned this pull request Jun 18, 2022

Logs: Add log record pooling #3385

Merged

1 task

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jun 26, 2022

cijothomas removed the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jun 27, 2022

cijothomas and others added 3 commits June 27, 2022 08:43

Merge branch 'main' into log-emitter

4f498af

Merge from main.

55537b5

Merge fix.

d1a4b3b

CodeBlanch changed the title ~~[SDK] LogRecordPool + LogEmitter API + Serilog/Tracing extensions~~ [SDK] LogEmitter API + Serilog/Tracing extensions Jun 30, 2022

CodeBlanch mentioned this pull request Jun 30, 2022

Logs: Add LogEmitter API #3423

Merged

1 task

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Jul 8, 2022

alanwest reviewed Jul 8, 2022

View reviewed changes

alanwest reviewed Jul 9, 2022

View reviewed changes

Merge from main.

ffda1ce

CodeBlanch changed the title ~~[SDK] LogEmitter API + Serilog/Tracing extensions~~ LogEmitter Serilog & Tracing extension projects Jul 11, 2022

CodeBlanch mentioned this pull request Jul 11, 2022

[Logs] Serilog extensions project #3438

Merged

2 tasks

CodeBlanch added 2 commits July 18, 2022 09:52

Merge from main.

12c92ad

Updates.

cf69b8c

CodeBlanch changed the title ~~LogEmitter Serilog & Tracing extension projects~~ LogEmitter System.Diagnostics.Tracing extension project Jul 18, 2022

CodeBlanch mentioned this pull request Jul 18, 2022

[Logs] OpenTelemetry.Extensions.EventSource extensions project #3454

Merged

2 tasks

CodeBlanch closed this Jul 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

LogEmitter System.Diagnostics.Tracing extension project #3305

LogEmitter System.Diagnostics.Tracing extension project #3305

CodeBlanch commented May 25, 2022 •

edited

Loading

codecov bot commented May 25, 2022 •

edited

Loading

reyang May 25, 2022

CodeBlanch May 26, 2022

CodeBlanch commented May 26, 2022

CodeBlanch May 26, 2022

reyang May 27, 2022

CodeBlanch May 29, 2022

github-actions bot commented Jun 26, 2022

github-actions bot commented Jul 8, 2022

alanwest Jul 8, 2022

alanwest Jul 8, 2022

alanwest Jul 9, 2022

CodeBlanch commented Jul 18, 2022

LogEmitter System.Diagnostics.Tracing extension project #3305

LogEmitter System.Diagnostics.Tracing extension project #3305

Conversation

CodeBlanch commented May 25, 2022 • edited Loading

Changes

Proposed Public API

TODOs

codecov bot commented May 25, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented May 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 26, 2022

github-actions bot commented Jul 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Jul 18, 2022

CodeBlanch commented May 25, 2022 •

edited

Loading

codecov bot commented May 25, 2022 •

edited

Loading