Skip to content
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

Merged
merged 2 commits into from
Nov 11, 2023

Conversation

puneet-traceable
Copy link
Contributor

@puneet-traceable puneet-traceable commented Nov 10, 2023

Description

This PR adds functions to set bodies in span in case bodies are already truncated.
Currently for body attributes SetTruncatedBodyAttribute and SetTruncatedEncodedBodyAttribute 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:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

Copy link

codecov bot commented Nov 10, 2023

Codecov Report

Merging #219 (e3beab5) into main (de9ef45) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            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              
Files Coverage Δ
sdk/instrumentation/bodyattribute/bodyattribute.go 100.00% <100.00%> (ø)

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

@puneet-traceable
Copy link
Contributor Author

@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.

@puneet-traceable puneet-traceable changed the title ENG-37132: Adding bodyAttribute functions to support already truncate… ENG-37132: Adding bodyAttribute functions to support already truncated bodies Nov 10, 2023
// 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) {
Copy link
Collaborator

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).

Copy link
Contributor Author

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

@ryanericson
Copy link
Collaborator

@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.

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 .truncated attribute i.e. it assumes truncation is handled outside.

@puneet-traceable
Copy link
Contributor Author

@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.

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 .truncated attribute i.e. it assumes truncation is handled outside.

yeah keeping the two but trying to reuse one by the other.

@ryanericson ryanericson merged commit 157e76c into main Nov 11, 2023
6 of 7 checks passed
@ryanericson ryanericson deleted the puneet branch November 11, 2023 15:22
}

// 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 {
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants