Geneva exporter: Support resource attributes in log exporter#3646
Geneva exporter: Support resource attributes in log exporter#3646rajkumar-rangaraj merged 20 commits intoopen-telemetry:mainfrom
Conversation
86ba956 to
44fd0df
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3646 +/- ##
==========================================
+ Coverage 71.52% 71.72% +0.19%
==========================================
Files 455 445 -10
Lines 17617 17624 +7
==========================================
+ Hits 12601 12640 +39
+ Misses 5016 4984 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
44fd0df to
ccfb2fd
Compare
mattsains
left a comment
There was a problem hiding this comment.
A while ago we decided that the resource attributes service.name and service.instanceId would be mapped to the message pack data even if they are not present in ResourceFieldNames. I was just thinking and realised that if someone had set cloud.role or cloud.roleInstance in PrepopulatedFields, then after my change, they will not have their logs exported, because the prepopulated field will conflict with the "built-in" resource attribute mapping for service.name and service.instanceId (which gets mapped to cloud.role and cloud.roleInstance even if you didn't specify it in ResourceFieldNames)
what do you think I should do? I can check to see if it's in resource attributes before I accept it as a prepopulated field, and then I guess just drop the prepopulated field?
I have updated the code and the tests to replace a prepopulated field with the equivalent auto-mapped resource attribute (ie., service.name, service.instanceId). I also added a test for this behaviour |
| if (this.prepopulatedFields != null) | ||
| { | ||
| for (var i = 0; i < this.prepopulatedFieldKeys.Count; i++) | ||
| foreach (var field in this.prepopulatedFields) |
There was a problem hiding this comment.
Existing implementation uses for loop to avoid the allocation from foreach.
There was a problem hiding this comment.
as discussed, this isn't a concern because this foreach is unlikely to have a heap allocation
There was a problem hiding this comment.
If I recall correctly, foreach inherently cause heap allocation.
It's a major goal to keep heap allocation as low as possible for Geneva exporter. Can you run a benchmark test in Benchmark folder to confirm there is no heap allocation introduced by foreach? (PR #1504 also included a mini benchmark code)
There was a problem hiding this comment.
I can't find any benchmarking code in that PR but if you can point me to a specific benchmark test I'd be happy to run it and post the results here
There was a problem hiding this comment.
There are two places you can reference:
- Ctrl+F "Log Benchmarks Code" on page [Exporter.Geneva] Perf improvement on .NET 8 #1504 and click on it
There was a problem hiding this comment.
Oops, I don't know how I missed that collapseable section in the PR.
Here's the results of the benchmark on my branch:
| Method | IncludeFormattedMessage | Mean | Error | StdDev | Gen0 | Allocated |
|---|---|---|---|---|---|---|
| LoggerWithMessageTemplate | False | 944.3 ns | 8.32 ns | 7.78 ns | - | 104 B |
| LoggerWithDirectLoggerAPI | False | 1,136.2 ns | 10.99 ns | 8.58 ns | 0.0057 | 320 B |
| LoggerWithSourceGenerator | False | 933.9 ns | 11.04 ns | 10.32 ns | - | 64 B |
| SerializeLogRecord | False | 316.7 ns | 1.98 ns | 1.76 ns | - | - |
| Export | False | 638.5 ns | 9.73 ns | 7.59 ns | - | - |
| LoggerWithMessageTemplate | True | 961.9 ns | 8.06 ns | 7.54 ns | 0.0010 | 104 B |
| LoggerWithDirectLoggerAPI | True | 1,117.1 ns | 6.50 ns | 5.76 ns | 0.0057 | 320 B |
| LoggerWithSourceGenerator | True | 929.5 ns | 8.82 ns | 8.25 ns | - | 64 B |
| SerializeLogRecord | True | 315.9 ns | 0.87 ns | 0.73 ns | - | - |
| Export | True | 641.1 ns | 7.51 ns | 5.86 ns | - | - |
There was a problem hiding this comment.
I also ran the tests on main branch. This confirmed that the PR didn't increase memory allocation. Interestingly, the memory allocation for .NET 10 is higher than .NET 8 (both on main branch):
.NET 8
| Method | IncludeFormattedMessage | Mean | Error | StdDev | Gen0 | Allocated |
|---|---|---|---|---|---|---|
| LoggerWithMessageTemplate | False | 1,142.1 ns | 22.74 ns | 42.16 ns | 0.0038 | 104 B |
| LoggerWithDirectLoggerAPI | False | 1,331.4 ns | 26.41 ns | 36.15 ns | 0.0114 | 320 B |
| LoggerWithSourceGenerator | False | 1,122.7 ns | 17.91 ns | 28.41 ns | 0.0019 | 64 B |
| SerializeLogRecord | False | 434.3 ns | 8.42 ns | 8.27 ns | - | - |
| Export | False | 773.8 ns | 15.53 ns | 35.04 ns | - | - |
| LoggerWithMessageTemplate | True | 1,132.5 ns | 22.65 ns | 44.71 ns | 0.0038 | 104 B |
| LoggerWithDirectLoggerAPI | True | 1,285.8 ns | 24.61 ns | 23.02 ns | 0.0114 | 320 B |
| LoggerWithSourceGenerator | True | 1,129.4 ns | 22.21 ns | 24.69 ns | 0.0019 | 64 B |
| SerializeLogRecord | True | 434.2 ns | 5.31 ns | 4.71 ns | - | - |
| Export | True | 769.2 ns | 15.25 ns | 31.84 ns | - | - |
.NET 10
| Method | IncludeFormattedMessage | Mean | Error | StdDev | Median | Gen0 | Allocated |
|---|---|---|---|---|---|---|---|
| LoggerWithMessageTemplate | False | 1,042.3 ns | 20.88 ns | 60.58 ns | 1,045.5 ns | 0.0038 | 112 B |
| LoggerWithDirectLoggerAPI | False | 1,242.6 ns | 24.58 ns | 47.36 ns | 1,222.2 ns | 0.0114 | 320 B |
| LoggerWithSourceGenerator | False | 1,063.2 ns | 21.24 ns | 44.33 ns | 1,051.1 ns | 0.0019 | 64 B |
| SerializeLogRecord | False | 383.3 ns | 7.46 ns | 9.70 ns | 379.8 ns | - | - |
| Export | False | 745.4 ns | 14.88 ns | 32.35 ns | 741.4 ns | - | - |
| LoggerWithMessageTemplate | True | 1,102.1 ns | 31.75 ns | 92.60 ns | 1,082.8 ns | 0.0038 | 112 B |
| LoggerWithDirectLoggerAPI | True | 1,219.1 ns | 24.17 ns | 33.08 ns | 1,213.6 ns | 0.0114 | 320 B |
| LoggerWithSourceGenerator | True | 1,069.4 ns | 23.99 ns | 69.61 ns | 1,065.7 ns | 0.0019 | 64 B |
| SerializeLogRecord | True | 369.3 ns | 4.97 ns | 4.41 ns | 368.6 ns | - | - |
| Export | True | 763.3 ns | 15.09 ns | 41.57 ns | 755.1 ns | - | - |
There was a problem hiding this comment.
Interesting, could you look into where the delta is coming from in .NET 10 comparing to 8?
There was a problem hiding this comment.
The 8 bytes come from the size increase of Microsoft.Extensions.Logging.FormattedLogValues, which is created via new in LoggerExtensions.LogInformation.
I checked the source code: in .NET 10, an additional field _cachedToString of type string? was introduced in FormattedLogValues: .NET 10 code vs .NET 8 code. This field is a reference, so it's 8 bytes on 64-bit CLR.
|
We talked more about the prepopulated/resource attribute thing, and decided that the mapping can only happen when prepopulated fields have not been specified. Otherwise, the default resource will silently overwrite prepopulated values. |
src/OpenTelemetry.Exporter.Geneva/Internal/MsgPack/MsgPackLogExporter.cs
Outdated
Show resolved
Hide resolved
| if (this.prepopulatedFields != null) | ||
| { | ||
| for (var i = 0; i < this.prepopulatedFieldKeys.Count; i++) | ||
| foreach (var field in this.prepopulatedFields) |
There was a problem hiding this comment.
If I recall correctly, foreach inherently cause heap allocation.
It's a major goal to keep heap allocation as low as possible for Geneva exporter. Can you run a benchmark test in Benchmark folder to confirm there is no heap allocation introduced by foreach? (PR #1504 also included a mini benchmark code)
Co-authored-by: Reiley Yang <reyang@microsoft.com>
| if (useMsgPackExporter) | ||
| { | ||
| var msgPackLogExporter = new MsgPackLogExporter(options); | ||
| var msgPackLogExporter = new MsgPackLogExporter(options, () => |
There was a problem hiding this comment.
Should this have a null check? If ParentProvider hasn't been set when export is called (possible during SDK initialization edge cases), this will throw NRE. Consider:
() => this.ParentProvider?.GetResource() ?? Resource.Empty
There was a problem hiding this comment.
I'll change it but I notice that if ParentProvider can really be null, then the existing metric code is also broken: https://github.com/open-telemetry/opentelemetry-dotnet-contrib/blob/main/src/OpenTelemetry.Exporter.Geneva/Metrics/GenevaMetricExporter.cs#L112
There was a problem hiding this comment.
What does this mean? Is it a bug or not?
There was a problem hiding this comment.
This is not a bug; the wording may have caused the confusion.
In the existing implementation, this.ParentProvider.GetResource() itself already returns Resource.Empty when no provider is set, so it is safe. We don’t need to add an explicit null check here or manually set Resource.Empty again.
There was a problem hiding this comment.
Thanks @rajkumar-rangaraj! Then this sounds wrong and should be reverted back.
I'll change it
There was a problem hiding this comment.
Yes, this needs a fix. I will send a follow-up PR.
3d6037a to
1469019
Compare
Related to #3214
Changes
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes