[sdk-logs] Support enrichment of LogRecords

[CodeBlanch]

[Opening as a draft to discuss if this is something we want to do.]

Changes

• Adds AddAtribute API on LogRecord to give users wanting to do enrichment a very efficient way to do it

Merge requirement checklist

• CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
• Unit tests added/updated
• Appropriate CHANGELOG.md files updated for non-trivial changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 85.21%. Comparing base (6250307) to head (6acac91).
Report is 202 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5454      +/-   ##
==========================================
+ Coverage   83.38%   85.21%   +1.82%     
==========================================
  Files         297      290       -7     
  Lines       12531    12456      -75     
==========================================
+ Hits        10449    10614     +165     
+ Misses       2082     1842     -240     

Flag	Coverage Δ
unittests	?
unittests-Instrumentation-Experimental	24.19% <0.00%> (?)
unittests-Instrumentation-Stable	24.23% <0.00%> (?)
unittests-Solution-Experimental	84.91% <0.00%> (?)
unittests-Solution-Stable	85.17% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/OpenTelemetry/Logs/LogRecord.cs	65.66% <0.00%> (-4.08%)	⬇️
.../OpenTelemetry/Logs/LogRecordEnrichedAttributes.cs	0.00% <0.00%> (ø)

... and 63 files with indirect coverage changes

[cijothomas]

Haven't checked the actual code, but a quick thought.
Should we explore and confirm the ILogger's native support for this? ILogger is introducing dynamic filtering, so it makes sense if enrichment of Logs are also handled in ILogger itself.

https://github.com/dotnet/extensions/tree/main/src/Libraries/Microsoft.Extensions.Telemetry.Abstractions#log-enrichment this looks ready, but haven't tested myself.

[cijothomas]

@dariusclay to comment on plans for https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Extensions.Enrichment as well. We should try and keep a single solution, unless there is a strong reason to have multiple...

[CodeBlanch]

@cijothomas

To add some context here...

The use case I got from @vishweshbankwar is for AzureMonitor which wants to modify this processor to be something like...

        public override void OnEnd(LogRecord logRecord)
        {
            if (logRecord.SpanId == default || logRecord.TraceFlags == ActivityTraceFlags.Recorded)
            {
                logRecord.AddAttribute(new KeyValuePair<string, object?>("azureMonitor.samplingRate", value));

                base.OnEnd(logRecord);
            }
        }

Some thoughts...

SDK supports enrichment today because LogRecord.Attributes has public get/set (as does most everything on LogRecord) but the contract is IReadOnlyList which makes it tricky. Worse case users will make a complete copy of the list and replace it (allocation + cpu cost). Best case they will do what this PR does and wrap the list but they will still need to pay for an allocation. What this PR does is give a nice cheap (no allocation after pool warms up, very minor CPU hit) way to do it efficiently. Also a potential mechanism the SDK itself could utilize in the future to add its own attributes instead of adding properties to LogRecord and asking exporters to work with them.

• Could AzureMonitor use Microsoft.Extensions.Telemetry.Abstractions or OpenTelemetry.Extensions.Enrichment? Perhaps, but I doubt the distro wants to be that opinionated.

  • OpenTelemetry.Extensions.Enrichment doesn't support logging at the moment. When it does, it will face the same performance issues as above. It could use this proposed API though to accomplish its goal.
  • Microsoft.Extensions.Telemetry.Abstractions does support enrichment, but it also completely replaces the ILoggerFactory with its own implementation. Not sure if AzureMonitor could/would want to do that.

• Should we look at ILogger doing enrichment? Yes! I did a prototype myself a while back 🤣 Most logging frameworks (serilog, nlog, log4net, etc.) have their own enrichment APIs. IMO these are user-facing APIs for the devs setting up their logging pipelines. That is kind of a different thing though. This PR is more of a low-level thing for SDK components (really processors). But aspiring devs can build rich, fully-featured, user-facing frameworks on top of it for fun and profit 💸

[cijothomas]

@CodeBlanch I understand the desire to have a Activity like way to enrich via processors. It maybe desirable to do it that way to keep things consistent across logs and traces.
However, I'd prefer to confirm ILogger itself does not have plans to add this. (in ILogger itself, via scope/its-variations, via ms.ext.telemetry.abstractions).

ILogger's own mechanism would work in providers outside of OpenTelemetry, like the planned Filtering mechanisms for ILogger. ILogger didn't have dynamic filtering till now, so each providers offered their own mechanism. But if there is a chance to settle on ILogger itself for enrichment, i'd suggest we evaluate that.

[reyang]

I suggest that we should first align on where enrichment should happen, and discuss this with the owners of ILogger and Microsoft.Extensions.Telemetry.Abstractions, so we don't end up with 3 confusing mechanisms while each solving 80% of the problems.

One approach would be documenting the end goal / experience before the actual implementation https://github.com/open-telemetry/opentelemetry-dotnet/tree/main/docs/logs#log-enrichment

[CodeBlanch]

Alignment? 🤔

LogRecordProcessor

...

The SDK MUST allow users to implement and configure custom processors and decorate built-in processors for advanced scenarios such as enriching with attributes.

We MUST support specifically enrichment via processors. Which we already do. The question is: Do we want to make it efficient and easy to do or no?

[github-actions]

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]

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.

[dariusclay]

@dariusclay to comment on plans for https://github.com/open-telemetry/opentelemetry-dotnet-contrib/tree/main/src/OpenTelemetry.Extensions.Enrichment as well. We should try and keep a single solution, unless there is a strong reason to have multiple...

The extensions mentioned here were to bridge the gap in current .NET 8 support for trace enrichment. I will try to get an update but from last time this was spoken about internally the goal was to have proper trace enrichment natively inside .NET 9.

[dariusclay]

• Microsoft.Extensions.Telemetry.Abstractions does support enrichment, but it also completely replaces the ILoggerFactory with its own implementation. Not sure if AzureMonitor could/would want to do that.
• Should we look at ILogger doing enrichment? Yes! I did a prototype myself a while back 🤣 Most logging frameworks (serilog, nlog, log4net, etc.) have their own enrichment APIs. IMO these are user-facing APIs for the devs setting up their logging pipelines. That is kind of a different thing though. This PR is more of a low-level thing for SDK components (really processors). But aspiring devs can build rich, fully-featured, user-facing frameworks on top of it for fun and profit 💸

The "extended" logger is what enables redaction and enrichment.
https://github.com/dotnet/extensions/blob/main/src/Libraries/Microsoft.Extensions.Telemetry/Logging/ExtendedLoggerFactory.cs

[github-actions]

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]

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]

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]

Closed as inactive. Feel free to reopen if this PR is still being worked on.