Replace recordingSpan attributes implementation with map of attributes #2555
Conversation
Instead of an LRU strategy for cap-ing span attributes, comply with the specification and drop last added. This means the attributesmap type can be replaced with a simple map.
MrAlias
added
bug
MrAlias
changed the title
Do not depend on attributes being returned in a consistent order.
Codecov Report
@@ Coverage Diff @@
## main #2555 +/- ##
=====================================
Coverage 76.0% 76.0%
=====================================
Files 174 173 -1
Lines 12190 12170 -20
=====================================
- Hits 9268 9258 -10
+ Misses 2677 2669 -8
+ Partials 245 243 -2
|
This comment has been minimized.
This comment has been minimized.
MrAlias
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod,
paivagustavo,
pellared and
XSAM
as code owners
MrAlias
changed the title
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
| trace.WithAttributes(attribute.String("key2", "value2")), | ||
| trace.WithAttributes(attribute.String("key1", "value2")), |
There was a problem hiding this comment.
Why did you change this test?
There was a problem hiding this comment.
Because there are two attributes set originally, the order of those attributes is a something checked indirectly. That order is not guaranteed anymore.
This updates the test to still set two attributes, similar to the original, but it updates the key-value. Resulting in a single attribute for the exported span. This update behavior is something guaranteed and is functionality also required by the specification.
There was a problem hiding this comment.
That makes me think that our assert isn't comprehensive enough for our test cases.
But, we cover this behavior in another test, so I won't hold this up.
There was a problem hiding this comment.
This is a significant improvement, approving on the merits. For myself I'd argue to go even further.
I would optimize for StartSpan performance => keep a reference to the original []attribute.KeyValue and document that the caller MUST not modify that slice. If the slice length exceeds the maximum, call deduplicateAndDrop(). Do not check for duplicates, otherwise, just store the array.
The SetAttributes() method would be optimized to avoid deduplication in a hot path as well, if possible. As long as the total slice length will not exceed the limit, simply append(sp.attributes, newAttributes...) and defer the deduplication. If the total slice length exceeds the limit, call deduplicateAndDrop().
The call to attributesLocked() is when I would deduplicate. We don't need to allocate a map to deduplicate, we can sort in place, so the end result will be that if you only call StartSpan and never call SetAttributes, the SDK itself won't perform any allocations for span attributes.
I think that would be a significant change to the data ownership semantics and could break existing uses. I would not support making such a change without a major version bump. Making that change in the SDK also could lead to a situation where different SDK implementations behave differently and code that is correct against one implementation could be very incorrect against another. That seems like something we should try to avoid. |
Avoid the duplicate reference to the value held by the parent TracerProvider and just refer to that directly when needed.
| trace.WithAttributes(attribute.String("key2", "value2")), | ||
| trace.WithAttributes(attribute.String("key1", "value2")), |
There was a problem hiding this comment.
That makes me think that our assert isn't comprehensive enough for our test cases.
But, we cover this behavior in another test, so I won't hold this up.
I agree a change to the data ownership semantics is probably not something we want to do. However, I am intrigued at the possibility to use a slice here. If we can realize the same performance improvements shown here and potentially maintain order of the attributes according to how the user adds them it seems like something worth exploring. I put together an initial implementation of this and it looks worth developing into a full alternate approach and evaluating the merits. I will do so before merging this. |
Only allocate if attributes are recorded.
…etry-go into refactor-SetAttributes
|
I just noticed that with the removal of the attributeMap and its tests we have lost coverage that |
I don't see the existing test coverage, but I'm happy to add these tests. |
MrAlias
changed the title
|
Reverting to a draft so this doesn't accidentally get merged. I plan to present this and #2576 at the SIG meeting today and discuss which approach we should move forward with. |
|
Closing in favor of #2576 |
Instead of an LRU strategy for cap-ing span attributes, comply with the specification and drop last added. This means the
attributesMaptype can be replaced with a simple map.Resolves #2554 and relates to #2402
Testing Performance
This looks to have considerable memory and allocation reduction and faster computations.