[Exporter.Geneva] implement resource attributes for logs #3531
Conversation
github-actions
bot
added
comp:exporter.geneva
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3531 +/- ##
==========================================
+ Coverage 70.92% 70.95% +0.03%
==========================================
Files 430 430
Lines 17214 17234 +20
==========================================
+ Hits 12209 12229 +20
Misses 5005 5005
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
mattsains
force-pushed
the
logs-resources
branch
3 times, most recently
from
2f4d319 to
35ccac1
Compare
mattsains
force-pushed
the
logs-resources
branch
from
35ccac1 to
85e9c53
Compare
|
|
||
| Resources.Resource ResourceProvider() | ||
| { | ||
| return connectionStringBuilder.HonorResourceAttributes ? this.ParentProvider.GetResource() : Resources.Resource.Empty; |
There was a problem hiding this comment.
is this a simple boolean setting, which, when enabled, will add all resource attributes to the log?
There was a problem hiding this comment.
yes, it's a connection string-based opt-in feature switch
There was a problem hiding this comment.
Got it. I spoke to @rajkumar-rangaraj also, and IMHO, it should be a per-attribute opt-in instead of adding all resource attributes. I understand users can control Resource by limiting things added to Resource, but Resource is not per exporter, so it's possible they need full resource in another exporter but limited one in Geneva, which is impossible to achieve now.
Few reasons why I think this should be enabled on per-resource-attribute basis.
- Consistency with other Geneva - elsewhere, this is opt-in on per-attribute basis. eg: OTel Rust
- Given GenevaExporter can only operate with a local agent, it is much better for the agent to add most of the resource information itself. (This is quite different than OTLP approach where agent is likely remote and cannot deduce resource information itself)
- Payload size limitations - both etw and linux user_events transport have strict hard limit of 64KB, so adding all the resource attributes is not a good idea.
- Unlike OTLP/Zipkin exporters, GenevaExporter need to send Resource attribute with each event - this is already bad (but due to good reasons to make each event self-contained), and is better to selectively send a subset of attributes.
There was a problem hiding this comment.
does this comment apply to the other PR relating to traces? #3214
There was a problem hiding this comment.
I actually attempted to make the change you are suggesting for traces, but it received negative feedback so I decided not to pursue it: #3367 (comment)
There was a problem hiding this comment.
does this comment apply to the other PR relating to traces? #3214
Yes.
There was a problem hiding this comment.
I actually attempted to make the change you are suggesting for traces, but it received negative feedback so I decided not to pursue it: #3367 (comment)
Yes I am aware. I am asking Raj to reconsider, so as to be consistent with the prior work (OTel Rust, which has this as a stable feature already). OTel Rust did it the way it did for the reasons I shared in earlier comment.
|
|
||
| foreach (var entry in this.propertiesEntries) | ||
| { | ||
| // A prepopulated env_properties entry should not be added if the same key exists in the log, |
There was a problem hiding this comment.
GenevaExporter previously did not attempt de-duplication for performance reasons (neither did upstream OTel sdk). I don't think we need to do it now either as this will affect perf.
There was a problem hiding this comment.
it was previously not possible to have a duplicate in env_properties. Do you think I should remove this check for duplicates between resource attributes and log fields? How do I weigh sending bad data to the agents versus performance impact?
There was a problem hiding this comment.
For Logs, duplicates were possible before. (eg: #736).
The current behavior is to let Agent deal with the duplicate data. It may/may-not be ideal, but that is the current state. (IIRC, metrics also don't do de-duplication of attributes. Agent deals with de-duplication)
Kielek
changed the title
| private readonly Dictionary<string, object> prepopulatedFields; | ||
|
|
||
| // These are values that are always added to env_properties | ||
| private readonly Dictionary<string, object> propertiesEntries; |
There was a problem hiding this comment.
FrozenDictionary is optimized for read-heavy scenarios with zero-allocation lookups and perfect hashing. Switching to regular Dictionary adds a lookup overhead
| var idxMapSizePatch = cursor - 2; | ||
|
|
||
| if (this.prepopulatedFieldKeys != null) | ||
| this.AddResourceAttributesToPrepopulated(); |
There was a problem hiding this comment.
This method runs on every log record, which is concerning for the hot path. While the idempotency guards prevent duplicate additions, we're still invoking the resourceProvider delegate, iterating all resource attributes, and performing dictionary lookups per-record. Additionally, the .Any() calls allocate an enumerator and closure on each invocation.
3 participants
Changes
This follows up on #3214 by implementing similar behaviour for including resource attributes in Geneva Logs.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial changes