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

Logs: Scope improvements #2026

Merged
merged 17 commits into from
May 6, 2021
Merged

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented May 3, 2021

Issue 1: Scopes + Batching

There is an issue in the current scope API where if you capture a LogRecord, say for batching, the active scopes may have changed by the time the exporter runs.

  • LogRecord.BufferLogScopes has been introduced to copy the active scopes into a buffer which can be inspected outside of the log lifecycle. This is now used by the BatchLogRecordExportProcessor. Made opt-in because a processor like SimpleLogRecordExportProcessor doesn't need the buffering and can use the ForEachScope call directly in the allocation-free form provided by the .NET API. LogRecord.ScopeProvider is now cleared after Processor.OnEnd is called to prevent accidental misuse.

Issue 2: Working with Scopes

  • The API today exposes the ability to loop over the scopes as objects. To be slightly more helpful we also now expose an enumerator which will return the individual KeyValuePair<string, object>s inside each scope. This makes it more aligned with the state parsing mechanism already in there. Can be done without allocation.

Usage Example

Sub-item Enumeration:

                logRecord.ForEachScope(ProcessScopeItem, this);

                static void ProcessScopeItem(LogRecordScope scope, ConsoleLogRecordExporter exporter)
                {
                    foreach (KeyValuePair<string, object> scopeItem in scope)
                    {
                        exporter.WriteLine($"[Scope]:{scopeItem.Key.PadRight(RightPaddingLength)}{scopeItem.Value}");
                    }
                }

API Changes

// Existing
public sealed class LogRecord
{
   // Signature changed.
   public void ForEachScope<TState>(Action<LogRecordScope, TState> callback, TState state);
}

// New
public readonly struct LogRecordScope
{
        public object Scope { get; }

        public Enumerator GetEnumerator() => new Enumerator(this.Scope);

        // For enumerating sub-items on the scope.
        public struct Enumerator : IEnumerator<KeyValuePair<string, object>>
}

TODOs

  • Unit tests.
  • CHANGELOG.md updated for non-trivial changes
  • Changes in public API reviewed

@CodeBlanch CodeBlanch requested a review from a team May 3, 2021 06:20
@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #2026 (e3c5ecd) into main (c28efc3) will decrease coverage by 0.05%.
The diff coverage is 85.45%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2026      +/-   ##
==========================================
- Coverage   83.90%   83.84%   -0.06%     
==========================================
  Files         192      193       +1     
  Lines        6195     6247      +52     
==========================================
+ Hits         5198     5238      +40     
- Misses        997     1009      +12     
Impacted Files Coverage Δ
src/OpenTelemetry/BatchLogRecordExportProcessor.cs 0.00% <0.00%> (ø)
src/OpenTelemetry/Logs/LogRecordScope.cs 82.60% <82.60%> (ø)
src/OpenTelemetry/Logs/LogRecord.cs 93.84% <96.42%> (+1.34%) ⬆️
src/OpenTelemetry/Logs/OpenTelemetryLogger.cs 94.11% <100.00%> (+0.17%) ⬆️
...rc/OpenTelemetry/SimpleLogRecordExportProcessor.cs 0.00% <0.00%> (-100.00%) ⬇️
...lemetry/Internal/SelfDiagnosticsConfigRefresher.cs 86.91% <0.00%> (-2.81%) ⬇️
...ZPages/Implementation/ZPagesExporterEventSource.cs 62.50% <0.00%> (+6.25%) ⬆️

@CodeBlanch
Copy link
Member Author

Updates based on SIG discussion today:

  • BufferLogScopes is now internal.
  • Removed depth tracking from LogRecordScope.
  • Made LogRecordScope readonly struct instead of readonly ref struct and removed the delegate.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

Thanks!

@cijothomas cijothomas merged commit 6cdfcea into open-telemetry:main May 6, 2021
@CodeBlanch CodeBlanch deleted the log-scope-batching branch May 6, 2021 20:39
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.

5 participants