-
Notifications
You must be signed in to change notification settings - Fork 773
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
[logs] Mitigate unwanted object creation during configuration reload #5514
Merged
CodeBlanch
merged 19 commits into
open-telemetry:main
from
CodeBlanch:sdk-log-options-reload
Apr 12, 2024
Merged
Changes from 13 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
3c7f674
Fixes for log options reload.
CodeBlanch 38ddd36
Add code comments.
CodeBlanch 081a06f
CHANGELOG patch.
CodeBlanch ceaef66
Merge from main.
CodeBlanch 57ee199
Add trim annotations.
CodeBlanch 51ae4e9
Tweaks.
CodeBlanch 63194a2
Adjust CHANGELOG.
CodeBlanch 5c95b81
Revert changes to DelegatingOptionsFactory.
CodeBlanch e467ecc
CHANGELOG tweaks.
CodeBlanch d7022bb
Merge branch 'main' into sdk-log-options-reload
CodeBlanch 702274e
CHANGELOG tweaks.
CodeBlanch 0302dcd
Merge branch 'sdk-log-options-reload' of https://github.com/CodeBlanc…
CodeBlanch 2d99b73
Update src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs
CodeBlanch 686339e
Code review.
CodeBlanch a048557
Tweak CHANGELOG.
CodeBlanch a1c1acd
Revert "Tweak CHANGELOG."
CodeBlanch a1d7461
Revert "Code review."
CodeBlanch 57acafd
Code review.
CodeBlanch cbbbbaa
Merge branch 'main' into sdk-log-options-reload
CodeBlanch File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// Copyright The OpenTelemetry Authors | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
#nullable enable | ||
|
||
#if NET6_0_OR_GREATER | ||
using System.Diagnostics.CodeAnalysis; | ||
#endif | ||
|
||
namespace Microsoft.Extensions.Options; | ||
|
||
#if NET6_0_OR_GREATER | ||
internal sealed class SingletonOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor<TOptions> | ||
#else | ||
internal sealed class SingletonOptionsMonitor<TOptions> : IOptionsMonitor<TOptions> | ||
#endif | ||
where TOptions : class | ||
{ | ||
private readonly TOptions instance; | ||
|
||
public SingletonOptionsMonitor(IOptions<TOptions> options) | ||
{ | ||
this.instance = options.Value; | ||
} | ||
|
||
public TOptions CurrentValue => this.instance; | ||
|
||
public TOptions Get(string? name) => this.instance; | ||
|
||
public IDisposable? OnChange(Action<TOptions, string?> listener) => null; | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need all this infrastructure? Instead, can't you just use
IOptions
instead ofIOptionsMonitor
everywhere?cc @davidfowl @halter73 @tarekgh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is changing OTel to use
IOptions
(instead ofIOptionsMonitor
) forOpenTelemetryLoggerOptions
. But my thinking is, we can't prevent users from accessingIOptionsMonitor
and we can't prevent some future dev from re-introducing it. The infrastructure here is so we can make it deterministic and have unit tests validating it will work correctly shouldIOptionsMonitor<OpenTelemetryLoggerOptions>
sneak into the process anywhere.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IOptionsMonitor<OpenTelemetryLoggerOptions>
? What would they do with it?IOptionsMonitor
, don't they want updates as the config changes? That's why they choose IOptionsMonitor.Would it be better to just fail in this case if we explicitly want to block it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While it's unclear why anyone else would need to monitor
OpenTelemetryLoggerOptions
for changes when the core library won't react to changes itself, it feels like code smell to go out of our way prevent it by replacing core options services and changing how they work. What if someone wants to just new up anOpenTelemetryLoggerOptions
and callsoptions.AddProcessor(new BatchLogRecordExportProcessor(new OtlpLogExporter(new()))
in a test or something? Should they know that this will spawn a thread that will never get stopped?I think the core issue is how
BatchExportProcessor<T>
spawns a thread in its constructor.opentelemetry-dotnet/src/OpenTelemetry/BatchExportProcessor.cs
Lines 60 to 65 in 876e4fa
I know that this API has already shipped, so adding a
StartAsync
method or something like that may not be feasible, but could you unsealOnStart
in the base class and start the thread there? Or lazily start the thread some other way? If not, should we deprecate theBaseProcessor<LogRecord>
overload ofAddProcessor()
and tell people to use theFunc<IServiceProvider, BaseProcessor<LogRecord>
overload instead?If we have to override the
IOptionsMonitor<OpenTelemetryLoggerOptions>
, I agree with @eerhardt that it should throw from everything with aNotSupportedException
indicating that reloadingOpenTelemetryLoggerOptions
is completely unsupported.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed updates so that accessing
OpenTelemetryLoggerOptions
viaIOptionsMonitor
orIOptionsSnapshot
will result in aNotSupportedException
. Technically breaking, but I think the impact will be very low.This is the plan yes (more or less)! We have another API for building logging pipelines which does not suffer from these issues. The plan is to make that a stable API (#5442) and then we can
Obsolete
theseAddProcessor
methods onOpenTelemetryLoggerOptions
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good discussion and thanks for the suggestions/ideas!
While I agree with most parts here, I suggest that we focus on "mitigate the issue quickly with minimum change/risk" instead of trying to get a complete solution in this PR. I'm specifically concerned about throwing exception in this PR. I suggest that we take the feedback and think about exception or other approaches in a follow up PR once the mitigation/hotfix is released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that means just changing our references of
IOptionsMonitor<OpenTelemetryLoggerOptions>
=>IOptions<OpenTelemetryLoggerOptions>
? Don't do anything to prevent users from accessing IOptionsMonitor. Am I understanding that correctly?If so, I agree that since this is a hotfix, keep it scoped to resolving the issue at hand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have strong opinion regarding the actual solution (e.g. IOptions/IOptionsMonitor, singleton, etc.), as long as it meets the two conditions #5514 (review) I'm good 👍.