Skip to content
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

[log4net] bridge + trace context injection #3825

Merged
merged 27 commits into from
Dec 13, 2024

Conversation

lachmatt
Copy link
Contributor

@lachmatt lachmatt commented Nov 26, 2024

Why

Towards #3822

What

Initial implementation of log4net bridge and injection of trace context.

Bytecode instrumentation for log4net that adds appender exporting logs in OTLP using LogsApi.

Bytecode instrumentation targets and some of the implementation similar to Datadog's log4net DirectLogSubmission/trace context injection implementation. @zacharycmontoya PTAL and approve from Datadog's side.

Logs bridge is disabled by default. In order to enable it, OTEL_DOTNET_AUTO_LOGS_ENABLE_LOG4NET_BRIDGE env variable needs to be set.
Trace context injection is enabled by default.

I added an internal doc with a short description of instrumentation added in this PR, and known limitations.

If *Format overloads (e.g InfoFormat) are used for logging, format string is used as a body of a log record, and args are added as attributes. If additionally OTEL_DOTNET_AUTO_LOGS_INCLUDE_FORMATTED_MESSAGE is set, log4net.rendered_message attribute is added with a content of a formatted message. Sample output:

LogRecord.Timestamp:               2024-12-11T15:03:10.8476757Z
LogRecord.TraceId:                 277283fe7ba44f2e91b0cb0b2d5f5caf
LogRecord.SpanId:                  306922dc6adef3d6
LogRecord.TraceFlags:              Recorded
LogRecord.CategoryName:            TestApplication.Log4NetBridge.Program
LogRecord.Severity:                Info
LogRecord.SeverityText:            INFO
LogRecord.Body:                    {0}, {1} at {2:t}!
LogRecord.Attributes (Key:Value):
    log4net.rendered_message: Hello, world at 16:03!
    0: Hello
    1: world
    2: 12/11/2024 16:03:10

Notes:
Most of the LogsApi from OpenTelemetry .NET SDK is not public in non-rc builds. Due to this, and to achieve acceptable peformance, expressions are used to initialize delegates used for sending logs.

In order to avoid adding multiple appenders serving the same purpose, as a simple workaround for now, this integration is enabled only if ILogger integration is not initialized.

Tests

Included in PR.

Checklist

  • CHANGELOG.md is updated.
  • Documentation is updated.
  • New features are covered by tests.

@lachmatt
Copy link
Contributor Author

lachmatt commented Dec 5, 2024

I'm still looking into one problematic scenario, and extracting message template and args that were used for logging, but any feedback at this point would be welcome.

@lachmatt lachmatt marked this pull request as ready for review December 5, 2024 17:28
@lachmatt lachmatt requested a review from a team as a code owner December 5, 2024 17:28
.cspell/company-terms.txt Outdated Show resolved Hide resolved
@@ -3,6 +3,7 @@
<ItemGroup>
<PackageReference Include="Newtonsoft.Json" />
<PackageReference Include="Xunit.SkippableFact" />
<PackageReference Include="log4net" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would consider moving tests related to the dependencies to the separate package. To avoid brining here transitive dependencies.

test/IntegrationTests/Log4NetBridgeTests.cs Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@lachmatt lachmatt changed the title [log-bridge] log4net bridge [log4net] bridge + trace context injection Dec 11, 2024
@lachmatt
Copy link
Contributor Author

I added trace context injection, extraction of format string and args. I updated the PR description to better reflect scope of the changes in this PR.


> [!IMPORTANT]
> log4net bridge and trace context injection are experimental features.
> Both instrumentations can be disabled by setting `OTEL_DOTNET_AUTO_LOGS_LOG4NET_INSTRUMENTATION_ENABLED` to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the env variable OTEL_DOTNET_AUTO_LOGS_LOG4NET_INSTRUMENTATION_ENABLED anywhere, only OTEL_DOTNET_AUTO_LOGS_ENABLE_LOG4NET_BRIDGE. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two levels

  1. Injecting context to the log files by standard funtionality https://github.com/open-telemetry/opentelemetry-dotnet-instrumentation/pull/3825/files#diff-77b3519370ff48f6caf7c316f0ece966f2f0871c40e9381d451d2c5b1c76b40fR124
  2. Bridging everything to the exporter (typically OTLP) by OTEL_DOTNET_AUTO_LOGS_ENABLE_LOG4NET_BRIDGE

Copy link
Contributor

@zacharycmontoya zacharycmontoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, good job putting together the changes and the testing

@Kielek Kielek merged commit f92f503 into open-telemetry:main Dec 13, 2024
38 checks passed
@lachmatt lachmatt deleted the log4net-bridge branch December 13, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants