Skip to content

ENG-16503 Show request body for x-www-form-urlencoded HTTP requests #363

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

Merged
merged 5 commits into from
Jun 1, 2022

Conversation

bradAtTraceable
Copy link
Contributor

Description

Please include a summary of the change, motivation and context.

Testing

Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.

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

Documentation

Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.

@bradAtTraceable
Copy link
Contributor Author

Implemented the ability to capture request bodies for HTTP requests in the x-www-form-urlencoded format.

sb.append(",");
}

sb.append(String.format("\n \"%s\": \"%s\"", key, value));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't there be a nested json?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in this case; the data for x-www-form-urlencoded request bodies comes in as string-valued properties. However, I had put this code in here because I was having problems with snyk-scan complaining about a specific version of Jackson; originally, I used Jackson to create the JSON here, and I thought the problem might have something to do with my change, so I changed the implementation to construct the JSON manually. However, it turns out that the snyk-scan wasn't caused by my change; instead I fixed the problem by upgrading the Jackson version. I will go back and replace this code with a Jackson implementation.

return new ObjectMapper().writeValueAsString(obj);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
public static String convertToJSONString(Map<?, ?> map) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this change show request body for x-www-form-urlencoded HTTP requests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a utility method for converting string Maps to JSON. In order for Span attributes to show correctly in the UI, they must be in JSON format. However, as indicated in my other comment, I'll be changing the implementation to use Jackson to perform this conversion. I'll also add some Javadoc for this method.

import org.hypertrace.agent.core.instrumentation.buffer.BoundedCharArrayWriter;
import org.hypertrace.agent.core.instrumentation.buffer.ByteBufferSpanPair;
import org.hypertrace.agent.core.instrumentation.buffer.CharBufferSpanPair;
import org.hypertrace.agent.core.instrumentation.buffer.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this intentional or intellij? I think we should not use *

import org.hypertrace.agent.core.instrumentation.buffer.BoundedCharArrayWriter;
import org.hypertrace.agent.core.instrumentation.buffer.ByteBufferSpanPair;
import org.hypertrace.agent.core.instrumentation.buffer.CharBufferSpanPair;
import org.hypertrace.agent.core.instrumentation.buffer.*;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same imports here and in other files as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was Intellij

@bradAtTraceable bradAtTraceable merged commit d0c2e8c into main Jun 1, 2022
@bradAtTraceable bradAtTraceable deleted the ENG-16503 branch June 1, 2022 20:28
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.

2 participants