Enforce attribute limit#330
Conversation
|
+@vigneshtdev for review. |
Codecov Report
@@ Coverage Diff @@
## master #330 +/- ##
==========================================
- Coverage 95% 94.92% -0.08%
==========================================
Files 120 120
Lines 8202 8213 +11
Branches 730 732 +2
==========================================
+ Hits 7792 7796 +4
- Misses 410 417 +7
Continue to review full report at Codecov.
|
| this.activeTraceParams.numberOfAttributesPerSpan) { | ||
| const attributeKeyToDelete = | ||
| Object.keys(this.attributes.attributeMap).shift(); | ||
| delete this.attributes.attributeMap[attributeKeyToDelete]; |
There was a problem hiding this comment.
Why delete an existing attribute rather than just dropping the new attribute? Is that the specified way to do it?
There was a problem hiding this comment.
This implementation is based on the spec here: https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/TraceConfig.md#limits
When limits are exceeded, implementations should by default preserve the most recently added values and drop the oldest values.
There was a problem hiding this comment.
Thanks, makes sense.
There was a problem hiding this comment.
does shift() guarantee that it returns the oldest?
There was a problem hiding this comment.
As per my knowledge, integer properties are sorted, others (non-integer) are listed in the creation order. shift -> Removes the first element from an array. @draffensperger correct me, if I am wrong.
There was a problem hiding this comment.
I checked on the order by running the following JS in a node console:
x = {}
x.b = 2
x.a = 1
Object.keys(x) // gives ['b', 'a'] as expected
y = {}
y.a = 1
y.b = 2
Object.keys(y) // gives ['a', 'b'] as expected
That said, what would you think about adding a unit test to validate this "removes the last attribute" behavior?
There was a problem hiding this comment.
We do have unit test to cover this -> https://github.com/census-instrumentation/opencensus-node/pull/330/files#diff-0ab7878c00c0a6393dd66fd59e796156R337. Just wanted to make sure this is not passing by fluke.
| } | ||
|
|
||
| this.attributes.attributeMap[key] = value; | ||
| this.attributes.droppedAttributesCount = this.totalRecordedAttributes - |
There was a problem hiding this comment.
Would it work to just increment this.attributes.droppedAttributesCount in the above if block?
| /** Trace Parameters */ | ||
| activeTraceParams: configTypes.TraceParams; | ||
|
|
||
| private totalRecordedAttributes = 0; |
There was a problem hiding this comment.
If we increment droppedAttributeCount do we need this field?
mayurkale22
force-pushed
the
enforce_attributes_limit
branch
from
b64c88f to
28c7d05
Compare
| this.activeTraceParams.numberOfAttributesPerSpan) { | ||
| const attributeKeyToDelete = | ||
| Object.keys(this.attributes.attributeMap).shift(); | ||
| delete this.attributes.attributeMap[attributeKeyToDelete]; |
There was a problem hiding this comment.
does shift() guarantee that it returns the oldest?
| rootSpan.addAttribute('my_second_attribute', 'foo2'); | ||
| rootSpan.addAttribute('my_attribute_string', 'bar2'); | ||
| rootSpan.addAttribute('my_attribute_number', 456); | ||
| rootSpan.addAttribute('my_attribute_boolean', false); |
There was a problem hiding this comment.
Change the test to add same key again such that it is no longer oldest and hence should not be deleted.
draffensperger
left a comment
draffensperger
left a comment
There was a problem hiding this comment.
Makes sense to limit breaking changes where we can, so 👍 to keeping dropped attributes on Span.
5 participants
This PR will address the first part of #316