-
Notifications
You must be signed in to change notification settings - Fork 887
Logs: Add log record pooling #3385
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
Changes from 3 commits
3841023
2cd2d1a
06336bc
322aa60
8fcbd11
e425b21
bbd42c2
2b7cbce
cbcbf1a
56befa1
b4f95b5
da49e68
5b4d285
bf0db46
9b5133c
6565d3d
6b6a6ba
a1242dd
4000f85
c9dd94e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,9 +57,14 @@ public override void OnEnd(LogRecord data) | |
| // happen here. | ||
| Debug.Assert(data != null, "LogRecord was null."); | ||
|
|
||
| data!.BufferLogScopes(); | ||
| data!.Buffer(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lets add a TODO to add a targeted test for testing #2905
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had some tests I did on #2932 so I just updated and added them to this PR. |
||
|
|
||
| base.OnEnd(data); | ||
| LogRecordSharedPool.Current.TrackReference(data); | ||
|
|
||
| if (!this.TryExport(data)) | ||
| { | ||
| LogRecordSharedPool.Current.Return(data); | ||
|
utpilla marked this conversation as resolved.
|
||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,15 +30,21 @@ namespace OpenTelemetry.Logs | |
| public sealed class LogRecord | ||
| { | ||
| internal LogRecordData Data; | ||
| internal List<KeyValuePair<string, object?>>? AttributeStorage; | ||
| internal List<object?>? BufferedScopes; | ||
| internal int PoolReferenceCount = int.MaxValue; | ||
|
|
||
| private static readonly Action<object?, List<object?>> AddScopeToBufferedList = (object? scope, List<object?> state) => | ||
| { | ||
| state.Add(scope); | ||
| }; | ||
|
|
||
| private List<object?>? bufferedScopes; | ||
| internal LogRecord() | ||
| { | ||
| } | ||
|
|
||
| // Note: Some users are calling this with reflection. Try not to change the signature to be nice. | ||
| [Obsolete("Call LogRecordPool.Rent instead.")] | ||
| internal LogRecord( | ||
| IExternalScopeProvider? scopeProvider, | ||
| DateTime timestamp, | ||
|
|
@@ -191,9 +197,9 @@ public void ForEachScope<TState>(Action<LogRecordScope, TState> callback, TState | |
|
|
||
| var forEachScopeState = new ScopeForEachState<TState>(callback, state); | ||
|
|
||
| if (this.bufferedScopes != null) | ||
| if (this.BufferedScopes != null) | ||
| { | ||
| foreach (object? scope in this.bufferedScopes) | ||
| foreach (object? scope in this.BufferedScopes) | ||
| { | ||
| ScopeForEachState<TState>.ForEachScope(scope, forEachScopeState); | ||
| } | ||
|
|
@@ -213,22 +219,46 @@ internal ref LogRecordData GetDataRef() | |
| return ref this.Data; | ||
| } | ||
|
|
||
| internal void Buffer() | ||
| { | ||
| this.BufferLogStateValues(); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment/todo for "state" - do we need to do similar for state as well?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
| this.BufferLogScopes(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Buffers the state values attached to the log into a list so that | ||
| /// they can be safely processed after the log message lifecycle has | ||
| /// ended. | ||
| /// </summary> | ||
| private void BufferLogStateValues() | ||
| { | ||
| var stateValues = this.StateValues; | ||
| if (stateValues == null || stateValues == this.AttributeStorage) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| var attributeStorage = this.AttributeStorage ??= new List<KeyValuePair<string, object?>>(stateValues.Count); | ||
| attributeStorage.AddRange(stateValues); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add a comment this is what triggers the iteration/enumeration to "snap" what we need before things could get disposed.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
| this.StateValues = attributeStorage; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Buffers the scopes attached to the log into a list so that they can | ||
| /// be safely processed after the log message lifecycle has ended. | ||
| /// </summary> | ||
| internal void BufferLogScopes() | ||
| private void BufferLogScopes() | ||
| { | ||
| if (this.ScopeProvider == null || this.bufferedScopes != null) | ||
| if (this.ScopeProvider == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| List<object?> scopes = new List<object?>(); | ||
| List<object?> scopes = this.BufferedScopes ??= new List<object?>(16); | ||
|
utpilla marked this conversation as resolved.
Outdated
|
||
|
|
||
| this.ScopeProvider?.ForEachScope(AddScopeToBufferedList, scopes); | ||
| this.ScopeProvider.ForEachScope(AddScopeToBufferedList, scopes); | ||
|
|
||
| this.bufferedScopes = scopes; | ||
| this.ScopeProvider = null; | ||
| } | ||
|
|
||
| private readonly struct ScopeForEachState<TState> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.