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

Disallow LogRecord options to be updated after initialization. #2902

Closed
Yun-Ting opened this issue Feb 16, 2022 · 3 comments · Fixed by #3055
Closed

Disallow LogRecord options to be updated after initialization. #2902

Yun-Ting opened this issue Feb 16, 2022 · 3 comments · Fixed by #3055
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package

Comments

@Yun-Ting
Copy link
Contributor

Bug Report

Currently, the SDK allows LogRecord options to be changed after the object was being built by the LoggerFactory.
This could introduce unnecessary complexity toward different states (before/after options got updated) and making it harder to maintain.

Discussion: #2891 (comment)

What is the expected behavior?
Remove existing test code on changing the LogRecord options after the object was being built by the LoggerFactory in LogRecordTest.cs and OtlpLogExporterTests.cs`.

(To be discussed)
Explicitly disallow LogRecord options to be tweaked after instantiation.

@Yun-Ting Yun-Ting added the bug Something isn't working label Feb 16, 2022
@cijothomas
Copy link
Member

I would treat it as a bug that current LoggingProvider respected changes after building the factory. We need to "snapshot" the options, and not respect changes done after that. Only exception is adding processors, which is a documented/supported feature for traces, and we share same pipeline for logs as well, so need to keep it that for consistency.

@reyang reyang added help wanted Good for taking. Extra help will be provided by maintainers good first issue Good for newcomers logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package labels Feb 23, 2022
@TimothyMothra
Copy link
Contributor

TimothyMothra commented Mar 15, 2022

I'm interested in working on this.

I think this is an easy fix.
Can add this to the OpenTelemetryLoggerOptions class
internal OpenTelemetryLoggerOptions DeepCopy() => (OpenTelemetryLoggerOptions)this.MemberwiseClone();

and call this method in the OpenTelemetryLoggerProvider ctor
this.Options = options.DeepCopy();

This will clone all the boolean properties.
This won't clone any of reference-type fields, but these are only accessed once in the ctor and never again.

internal OpenTelemetryLoggerProvider(OpenTelemetryLoggerOptions options)
{
Guard.ThrowIfNull(options);
this.Options = options;
this.Resource = options.ResourceBuilder.Build();
foreach (var processor in options.Processors)
{
this.AddProcessor(processor);
}
}

@cijothomas
Copy link
Member

I would treat it as a bug that current LoggingProvider respected changes after building the factory. We need to "snapshot" the options, and not respect changes done after that. Only exception is adding processors, which is a documented/supported feature for traces, and we share same pipeline for logs as well, so need to keep it that for consistency.

^Correction to above. As of today, there is no ability to add processors after building the factory. This is different from tracing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Good for taking. Extra help will be provided by maintainers logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
4 participants