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

OTLP LogExporter to support ILogger Scopes #3218

Merged
merged 6 commits into from
Apr 21, 2022

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Apr 21, 2022

Towards : #2482
Focused on just getting the functionality and example usage.
Tests are not added, as the current test approach cannot be used for scopes, so this will be a separate PR.

Open qn:
For scopes, keys are prefixed with "[Scope.depth]:". This does require string allocated, but avoids collision when same keys are repeated in diff. scope depths... We can revisit this based on user feedback :)

For the code below:

            using (logger.BeginScope("My scope 1 with {food} and {color}", "apple", "green"))
            using (logger.BeginScope("My scope 2 with {food} and {color}", "banana", "yellow"))
            {
                logger.LogInformation("Hello from {name} {price}.", "tomato", 2.99);
            }

Sample output from OTLP collector

LogRecord #0
Timestamp: 2022-04-21 18:52:28.8868154 +0000 UTC
Severity: Information
ShortName:
Body: Hello from tomato 2.99.
Attributes:
     -> name: STRING(tomato)
     -> price: DOUBLE(2.99)
     -> {OriginalFormat}: STRING(Hello from {name} {price}.)
     -> [Scope.0]:food: STRING(apple)
     -> [Scope.0]:color: STRING(green)
     -> [Scope.0]:{OriginalFormat}: STRING(My scope 1 with {food} and {color})
     -> [Scope.1]:food: STRING(banana)
     -> [Scope.1]:color: STRING(yellow)
     -> [Scope.1]:{OriginalFormat}: STRING(My scope 2 with {food} and {color})

@cijothomas cijothomas requested a review from a team April 21, 2022 19:03
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #3218 (33db1ae) into main (60f743a) will decrease coverage by 0.50%.
The diff coverage is 25.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3218      +/-   ##
==========================================
- Coverage   85.74%   85.23%   -0.51%     
==========================================
  Files         260      252       -8     
  Lines        9358     9234     -124     
==========================================
- Hits         8024     7871     -153     
- Misses       1334     1363      +29     
Impacted Files Coverage Δ
...etryProtocol/Implementation/LogRecordExtensions.cs 87.65% <25.00%> (-6.87%) ⬇️
....TelemetryHttpModule/TelemetryHttpModuleOptions.cs 0.00% <0.00%> (-90.00%) ⬇️
...entation/ExportClient/OtlpGrpcTraceExportClient.cs 35.71% <0.00%> (-42.86%) ⬇️
...xporter.OpenTelemetryProtocol/OtlpTraceExporter.cs 36.36% <0.00%> (-40.91%) ⬇️
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 76.47% <0.00%> (-5.89%) ⬇️
....AspNet.TelemetryHttpModule/TelemetryHttpModule.cs 0.00% <0.00%> (-5.27%) ⬇️
...ter.ZPages/Implementation/ZPagesActivityTracker.cs 97.14% <0.00%> (-2.86%) ⬇️
...metryProtocol/Implementation/ActivityExtensions.cs 88.73% <0.00%> (-2.71%) ⬇️
...nTelemetry.Instrumentation.AspNet/AspNetMetrics.cs
...Implementation/AspNetInstrumentationEventSource.cs
... and 6 more

scopeDepth++;
foreach (var scopeItem in scope)
{
var scopeItemWithDepthInfo = new KeyValuePair<string, object>($"[Scope.{scopeDepth}]:{scopeItem.Key}", scopeItem.Value);
Copy link
Member

Choose a reason for hiding this comment

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

I anticipate users wanting to change this prefix behavior, but something we can tackle later 😄

Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to avoid the new string allocation each time, doesn't block this PR though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. The description has this left as open question.
Merging to make progress. (OTLP Log Exporter is now almost ready feature-wise, except the SDK level issue related to buffering : #2905)

Copy link
Member

@CodeBlanch CodeBlanch left a comment

Choose a reason for hiding this comment

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

LGTM

@cijothomas cijothomas merged commit 1918e4b into open-telemetry:main Apr 21, 2022
@cijothomas cijothomas deleted the cijothomas/otlp+log_scopes branch July 6, 2022 18:36
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.

3 participants