-
Notifications
You must be signed in to change notification settings - Fork 8
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
ENG-37132: Adding bodyAttribute functions to support already truncated bodies #219
Conversation
Codecov Report
@@ Coverage Diff @@
## main #219 +/- ##
==========================================
+ Coverage 58.94% 59.20% +0.25%
==========================================
Files 55 55
Lines 2236 2250 +14
==========================================
+ Hits 1318 1332 +14
Misses 859 859
Partials 59 59
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
@ryanericson @tim-mwangi not sure on our take of making breaking changes in this repo. I have added new functions for now, but if we are ok making breaking changes I can change existing functions. Also, will add tests based on inputs on these lines. |
// SetPossiblyTruncatedBodyAttribute sets the body as a span attribute. | ||
// When body is being truncated, or already truncated we also add a second attribute suffixed by `.truncated` to | ||
// make it clear to the user, body has been modified. | ||
func SetPossiblyTruncatedBodyAttribute(attrName string, body []byte, bodyMaxSize int, truncated bool, span sdk.Span) { |
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.
Does this one need to have truncation logic? It can just be used when client code wants to handle truncation on its own in which case no truncation logic is needed so function can just be setBodyAtttribute(attrName string, body []byte, truncated bool, span sdk.Span)
.
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.
updated and used the SetBodyAtttribute
from SetTruncatedBodyAttribute
It's ok to break compat but not sure if we need to go that way and function can be confusing IMO. Perhaps best to have 2 versions where one can do truncation inside, and another never does truncation but can put |
yeah keeping the two but trying to reuse one by the other. |
} | ||
|
||
// SetTruncatedEncodedBodyAttribute is like SetTruncatedBodyAttribute above but also base64 encodes the | ||
// body. This is usually due to non utf8 bytes in the body eg. for multipart/form-data content type. | ||
// The body attribute name has a ".base64" suffix. | ||
func SetTruncatedEncodedBodyAttribute(attrName string, body []byte, bodyMaxSize int, span sdk.Span) { | ||
bodyLen := len(body) | ||
if bodyLen == 0 { | ||
if len(body) == 0 { |
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.
why not use the variable bodyLen
?
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.
will fix it, missed it in a revert.
Description
This PR adds functions to set bodies in span in case bodies are already truncated.
Currently for body attributes
SetTruncatedBodyAttribute
andSetTruncatedEncodedBodyAttribute
functions are supported, but to use those users will need to read complete body and then let these functions do the truncation.to support reading limited sized bodies, new functions are added which truncated attribute.
Testing
Will be updating unit tests.
Checklist: