-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store span warnings as tags in Cassandra #4313
Store span warnings as tags in Cassandra #4313
Conversation
Signed-off-by: Utsav Oza <utsavoza96@gmail.com>
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4313 +/- ##
==========================================
+ Coverage 97.09% 97.10% +0.01%
==========================================
Files 300 300
Lines 17668 17700 +32
==========================================
+ Hits 17155 17188 +33
+ Misses 413 412 -1
Partials 100 100
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add tests for error handling paths that are not covered (see codecov comments)
Signed-off-by: Utsav Oza <utsavoza96@gmail.com>
kv, err := c.fromDBTag(&tags[i]) | ||
var retMe []model.KeyValue | ||
for _, tag := range tags { | ||
kv, err := c.fromDBTag(&tag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please change it back. Original version has no copying and no memory allocations, yours has both. And pre-allocating array of known size is usually more efficient because it avoids re-allocation as the vector grows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because your input and output lengths may not batch, you have to allocate with make(type, 0, len)
and use append
, not indexed assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Signed-off-by: Utsav Oza <utsavoza96@gmail.com>
Signed-off-by: Utsav Oza <utsavoza96@gmail.com>
@yurishkuro I don't think this solves/fixes #704 unless we add |
More I think about it, I think it does solve the issue. #704 doesn't actually explain the problem, it instead points to implementation details. I think the problem is that the Lines 63 to 68 in a631623
Since warnings previously were not part of the DB model, two identical domain spans that are only different in warnings would produce different hash code, even though when they are stored in the DB their stored image would be identical. I am not sure why even that would be a problem, sounds more like a theoretical issue that one would not come across in actual usage. But now that warnings are converted into special tags, the stored versions will be different, thus justifying the different hash codes, as was the intention. So feel free to keep the "resolves #704" in the description. |
Which problem is this PR solving?
Short description of the changes