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

Dependency update OpenTelemetry ver. 1.11.0-rc.1 #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snakefoot
Copy link
Contributor

@snakefoot snakefoot commented Dec 17, 2024

One might consider making this NLog target a wrapper around the public OpenTelemetryLoggerProvider.

Then "just" inject a special LogEvent-state-object that includes the NLog LogEventInfo-properties. Maybe consider making a pull-request so one can override the checking of global/static property Sdk.SuppressInstrumentation:

    public bool IsEnabled(LogLevel logLevel)
    {
        return logLevel != LogLevel.None && !Sdk.SuppressInstrumentation;
    }

@snakefoot snakefoot force-pushed the master branch 9 times, most recently from 1d56102 to dbc3ab4 Compare December 17, 2024 10:18
@juliuskoval
Copy link
Owner

I'm not sure what the purpose of this change is. The SpanId and TraceId fields of LogRecordData are populated whenever you create an instance.

As for your other suggestion, are you suggesting that we create an instance of OpenTelemetryLoggerProvider instead of LoggerProvider and use that to create instances of ILogger instead of OpenTelemetry.Logs.Logger? I think I considered that initially, but it looks like the public constructor for OpenTelemetryLoggerProvider will eventually be obsoleted when the API I'm using becomes stable.

I suppose we could use it as a temporary workaround until bridge API becomes stable, but I would rather avoid it. Either way, I asked the OpenTelemetry team here if they have an ETA on when the bridge API becomes stable, so we will see what they say.

@snakefoot snakefoot changed the title Added support for TraceId and SpanId Dependency update OpenTelemetry ver. 1.11.0-rc.1 Dec 21, 2024
@snakefoot
Copy link
Contributor Author

snakefoot commented Dec 21, 2024

I think I considered that initially, but it looks like the public constructor for OpenTelemetryLoggerProvider will eventually be obsoleted when the API I'm using becomes stable.

Believe it will always be possible to build a local Microsoft LoggerFactory (isolated instance inside OtlpTarget), and then add OpenTelemetry as Logging-Provider. Then let OtlpTarget perform "logging" to its own local Microsoft LoggerFactory.

I'm not sure what the purpose of this change is. The SpanId and TraceId fields of LogRecordData are populated whenever you create an instance.

Yes you are right. It was only because I saw OpenTelemetryLoggerProvider explicitly assign SpanId + TraceId, but I guess it is because it doing object-pool-rent.

I have now changed the pull-request to just be dependency update, and fixing of some build warnings.

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.

2 participants