[Logs] Serilog extensions project #3438

CodeBlanch · 2022-07-11T17:50:40Z

[Goal is to pare down the amount of changes on #3305]

Changes

Adds a project which exposes a sink for emitting OpenTelemetry logs written via Serilog

Notes

This project is currently part of the core repo because all of the LogEmitter artifacts are internal. Once those go public this should be moved to contrib repo.
The goal here is to release this project as an experimental/preview thing to gather feedback.

TODOs

Appropriate CHANGELOG.md updated for non-trivial changes
Unit tests

codecov · 2022-07-11T18:08:26Z

Codecov Report

Merging #3438 (3ec802a) into main (a19d106) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3438      +/-   ##
==========================================
- Coverage   86.37%   86.36%   -0.02%     
==========================================
  Files         263      265       +2     
  Lines        9550     9598      +48     
==========================================
+ Hits         8249     8289      +40     
- Misses       1301     1309       +8

Impacted Files	Coverage Δ
src/OpenTelemetry/Logs/LogEmitter.cs	`100.00% <ø> (ø)`
...tensions.Serilog/OpenTelemetrySerilogExtensions.cs	`100.00% <100.00%> (ø)`
...try.Extensions.Serilog/OpenTelemetrySerilogSink.cs	`100.00% <100.00%> (ø)`
...porter.OpenTelemetryProtocol/OtlpMetricExporter.cs	`72.72% <0.00%> (-13.64%)`	⬇️
...tation/OpenTelemetryProtocolExporterEventSource.cs	`85.00% <0.00%> (-10.00%)`	⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs	`76.47% <0.00%> (-5.89%)`	⬇️
...heus/Implementation/PrometheusCollectionManager.cs	`79.74% <0.00%> (-2.54%)`	⬇️
...metryProtocol/Implementation/ActivityExtensions.cs	`94.50% <0.00%> (-1.10%)`	⬇️
...Telemetry/Internal/SelfDiagnosticsEventListener.cs	`97.65% <0.00%> (+0.78%)`	⬆️
src/OpenTelemetry/Logs/LogRecordAttributeList.cs	`75.43% <0.00%> (+0.87%)`	⬆️
... and 1 more

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

src/OpenTelemetry.Extensions.Serilog/README.md

alanwest

LGTM. Just a couple thoughts that don't block anything.

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

alanwest · 2022-07-11T22:20:52Z

examples/LoggingExtensions/Examples.LoggingExtensions.csproj

+
+  <ItemGroup>
+    <ProjectReference Include="..\..\src\OpenTelemetry.Exporter.Console\OpenTelemetry.Exporter.Console.csproj" />
+    <ProjectReference Include="..\..\src\OpenTelemetry.Extensions.Serilog\OpenTelemetry.Extensions.Serilog.csproj" />


This name seems fine to me, but just thought I'd share the quick nuget search I did https://www.nuget.org/packages?q=serilog+sink. Appears most Serilog sinks have Sink in the package name.

nit: Use $(RepoRoot).

nit: Use $(RepoRoot).

Updated

Appears most Serilog sinks have Sink in the package name.

Right now the project is just a sink, but I don't know if it might contain other serilog artifacts in the future. For example say users are logging to flat files. They may want a serilog enricher that adds OTel context to those logs. We could also have that in this project. Basically my thinking was this project would just contain stuff to help with different otel/serilog scenarios. We could of course have dedicated packages for each thing. More overhead to do that. Don't feel strongly either way so if you want it renamed just LMK and I'll update.

Don't feel strongly either way so if you want it renamed just LMK and I'll update.

Nah, I don't think Sink is needed allowing for additional Serilog things in the future.

The other thing I was pondering from a package naming perspective is whether we consider this an Extension or something else... Java for example considers their support for log4j "instrumentation" https://github.com/open-telemetry/opentelemetry-java-instrumentation/tree/main/instrumentation/log4j.

I think we have more time to consider these things.

reyang · 2022-07-13T23:10:20Z

examples/LoggingExtensions/Program.cs

+
+// Note: It is important that OpenTelemetryLoggerProvider is disposed when the
+// app is shutdown. In this example we allow Serilog to do that by calling CloseAndFlush.
+var openTelemetryLoggerProvider = new OpenTelemetryLoggerProvider(options =>


Instead of assigning this to a variable openTelemetryLoggerProvider and explaining the object ownership/lifecycle, I suggest putting the new OpenTelemetryLoggerProvider expression directly in Ln:35 WriteTo.OpenTelemetry(new OpenTelemetryLoggerProvider(...), disposeProvider: true): it increases the locality when folks read the code, making the code more self-contained rather than highly contextual, it avoids accidental misuse (e.g. code holding a reference to disposed object).

Here is the version from the other PR: https://github.com/open-telemetry/opentelemetry-dotnet/pull/3305/files#diff-eebf3c37db933ef5eddad43dc03d2553b800b073573f291b7e0ac5999a1391c8

Where I am heading with this is two different types of extensions sharing a provider.

So the question is, what do we want to demo in the example? Sharing a provider or independent providers? I feel like shared will be the more common case. But LMK your thoughts on this.

Here is my take:

examples/LoggingExtensions/Program.cs looks an example for the end-user - who is using the OpenTelemetry.Extensions.Serilog package to pipe Serilog via OpenTelemetry plumbing. For these users, normally they will only use Serilog in a single application, it makes more sense for them to have something simple, and not to worry about object lifecycle management except for the top level CloseAndFlush.

examples/LogEmitter/Program.cs is more targeting the plumbers, might make sense to expose more complexity / duty.

@reyang Let me merge and then I'll follow-up on this on my next PR where the example project is going to get more interesting?

src/OpenTelemetry.Extensions.Serilog/OpenTelemetry.Extensions.Serilog.csproj

examples/LoggingExtensions/Program.cs

CodeBlanch · 2022-07-14T20:48:05Z

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

+                        // Note: The goal here is to build a typed array (eg
+                        // int[]) if all the element types match otherwise
+                        // fallback to object[]


@alanwest It was a bit tricky, but we now build a typed array when we can.

Interesting, yea looks like this does the thing. Going out on a limb here, but I'd suspect more often than not it'll all be of one type. So, most folks won't hit the perf penalty.

I didn't stress the perf too much. For two reasons. The first is I'm assuming logging arrays is kind of a rare thing? The second is Serilog is already destroying perf 🤣 Whatever you log it allocates/wraps/boxes into a spiderweb of reference types. Joined the darkside Serilog has.

🕷️ 🕸️ 🕶️ 🕸️ 🕷️

alanwest · 2022-07-14T23:00:56Z

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

+                        {
+                            if (elements[i] is ScalarValue value)
+                            {
+                                Type currentElementType = value.Value?.GetType() ?? typeof(object);


If this were of type MyDisposableType then later when this log record is exported it'd throw an object disposed exception when attempting to ToString the object. The exporters catch and swallow this kind of thing today.

We don't do anything special earlier in the pipeline for attributes on traces or metrics, but is there any reason we'd want to consider coercing things into one of the primitive types earlier for log data?

Not totally following you. But I don't think a complex type would end up as a ScalarValue.

I have this test at the moment:

opentelemetry-dotnet/test/OpenTelemetry.Extensions.Serilog.Tests/OpenTelemetrySerilogSinkTests.cs

Line 155 in 1643b31

Log.Logger.Information("Hello {greeting} {id} {@complexObj} {$complexStr}", "World", 18, complexType, complexType);

That is verifying a "destructed" complex type is currently dropped on the floor, because we don't yet have support for that. What happens in that case is instead of a ScalarValue you get a StructureValue which itself contains all the properties of the complex type represented as ScalarValues or other StructureValues, etc. I don't think disposable/mutable things are a concern because Serilog will capture all the data from the complex type and then represent it using these "value" types it has in here: https://github.com/serilog/serilog/tree/dev/src/Serilog/Events

Right now we have basic support. You can use Serilog to do structured log messaging with primitives + arrays of primitives and it will work into OTel. My goal is to get support into OTel somehow for complex types and then come back to this and properly support fully Serilog's destructuring mechanism 😄

Ah thanks I missed that aspect of the test. Definitely seems like a bridge to cross later once complex types are supported. If serilog does all the work then my question is moot. I wasn't sure if a complex type could end up being added to the attributes of a log record.

src/OpenTelemetry.Extensions.Serilog/OpenTelemetry.Extensions.Serilog.csproj

src/OpenTelemetry.Extensions.Serilog/README.md

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs

CodeBlanch added 2 commits July 11, 2022 10:18

Add Serilog extensions project.

6be30ae

Improve provider disposal when using serilog.

9c95f5a

CodeBlanch requested a review from a team July 11, 2022 17:50

Warning fixes.

6d940c0

cijothomas reviewed Jul 11, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jul 11, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/README.md Show resolved Hide resolved

Fix README.

69efea6

alanwest approved these changes Jul 11, 2022

View reviewed changes

Code review.

c35e24a

reyang reviewed Jul 13, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/OpenTelemetry.Extensions.Serilog.csproj Outdated Show resolved Hide resolved

reyang approved these changes Jul 13, 2022

View reviewed changes

CodeBlanch added 3 commits July 14, 2022 11:19

Move Serilog version to props.

0d6f211

Rename LogEmitter.Log -> Emit.

dd0a846

Merge branch 'main' into logemitter-serilog-extensions

51f7e6b

reyang reviewed Jul 14, 2022

View reviewed changes

examples/LoggingExtensions/Program.cs Outdated Show resolved Hide resolved

Added unit tests.

6e4bc28

CodeBlanch commented Jul 14, 2022

View reviewed changes

Warning cleanup.

1643b31

alanwest reviewed Jul 14, 2022

View reviewed changes

CodeBlanch added 2 commits July 15, 2022 09:59

Code review.

4c1d5ef

Merge branch 'main' into logemitter-serilog-extensions

1acf06c

cijothomas reviewed Jul 16, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/OpenTelemetry.Extensions.Serilog.csproj Outdated Show resolved Hide resolved

cijothomas reviewed Jul 16, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/README.md Show resolved Hide resolved

cijothomas reviewed Jul 16, 2022

View reviewed changes

src/OpenTelemetry.Extensions.Serilog/OpenTelemetrySerilogSink.cs Show resolved Hide resolved

cijothomas approved these changes Jul 16, 2022

View reviewed changes

CodeBlanch added 2 commits July 18, 2022 09:22

Standard README jazz.

350c1a4

Removed ApiCompat block.

3ec802a

CodeBlanch merged commit ab10e11 into open-telemetry:main Jul 18, 2022

CodeBlanch deleted the logemitter-serilog-extensions branch July 18, 2022 16:38

alanwest mentioned this pull request Jul 21, 2022

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

Merged

2 tasks

This was referenced Sep 29, 2022

[Logs] Revert logging API additions #3702

Merged

[Logs-branch] Add LoggerProvider spec changes #3707

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Logs] Serilog extensions project #3438

[Logs] Serilog extensions project #3438

CodeBlanch commented Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 11, 2022 •

edited

Loading

alanwest left a comment

alanwest Jul 11, 2022

utpilla Jul 12, 2022

CodeBlanch Jul 12, 2022

alanwest Jul 12, 2022

reyang Jul 13, 2022

CodeBlanch Jul 14, 2022

reyang Jul 14, 2022

CodeBlanch Jul 15, 2022 •

edited

Loading

CodeBlanch Jul 14, 2022

alanwest Jul 14, 2022

CodeBlanch Jul 15, 2022

alanwest Jul 15, 2022

alanwest Jul 14, 2022

CodeBlanch Jul 15, 2022

alanwest Jul 15, 2022

[Logs] Serilog extensions project #3438

[Logs] Serilog extensions project #3438

Conversation

CodeBlanch commented Jul 11, 2022 • edited Loading

Changes

Notes

TODOs

codecov bot commented Jul 11, 2022 • edited Loading

Codecov Report

alanwest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeBlanch commented Jul 11, 2022 •

edited

Loading

codecov bot commented Jul 11, 2022 •

edited

Loading

CodeBlanch Jul 15, 2022 •

edited

Loading