Enforce Links limit #331
Enforce Links limit #331
Conversation
Codecov Report
@@ Coverage Diff @@
## master #331 +/- ##
==========================================
- Coverage 94.95% 94.95% -0.01%
==========================================
Files 120 120
Lines 8213 8281 +68
Branches 732 737 +5
==========================================
+ Hits 7799 7863 +64
- Misses 414 418 +4
Continue to review full report at Codecov.
|
| {value: 'boolValue', boolValue: true} | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
you have checked the drop counts for links earlier but it would nice to check that oldest links is removed. you can either add this is as part of this or where you test the dropLinkCount.
There was a problem hiding this comment.
Good Point! We already have asset check here https://github.com/census-instrumentation/opencensus-node/pull/331/files#diff-0ab7878c00c0a6393dd66fd59e796156R482 for the links. Basically, it doesn't contain a dropped link object.
There was a problem hiding this comment.
as per our conversation, this is already taken care of in previous test 'should adapt a span correctly' on line 320.
so ignore the comment.
| 'my_annotation', | ||
| {myString: 'bar', myNumber: 123, myBoolean: true}); | ||
|
|
||
| // Metric Event |
There was a problem hiding this comment.
Nit: "Message event"
|
|
||
| // Metric Event | ||
| const timeStamp = 123456789; | ||
| rootSpan.addMessageEvent('MessageEventTypeSent', 'ffff', timeStamp); |
There was a problem hiding this comment.
Curious, why is this 'MessageEventTypeSent' and not just 'SENT'?
This would be for a follow up PR, but what would you think about adding enums for span kind, message event type and link type?
There was a problem hiding this comment.
This is legacy code, I am not sure rational behind this. Created an issue to track this -> #333. Thanks 👍
This PR will address the second part (enforce limits on links) of #316