Skip to content

Capture text-based request bodies #498

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 20 commits into from
Mar 22, 2019
Merged

Capture text-based request bodies #498

merged 20 commits into from
Mar 22, 2019

Conversation

@felixbarny
Copy link
Member

@felixbarny felixbarny commented Feb 21, 2019 •
edited by alvarolobato
Loading

closes #449

d4294 and others added 14 commits January 21, 2019 14:59
Initial commit for POC that request body can be send to APM server
efa2ac9
Started implementing comments
672d561
Implemented change requests as stated in github comments
a3cdc19
Fixed wrong implementation of onReadExit. Parameters of read method h…
59e0ed1 
…ad been interpreted wrong
Extended InputStreamInstrumentation to be able to instrument ReadLine…
16a7e95 
… method as well
Merge branch 'master' of https://github.com/elastic/apm-agent-java in…
9b1e5e1 
…to improvement/capture-request-body

Merge source repository with fork
Continued to implement comments of felixbarny
010bb4c
Use prepared body request capture infrastructure
1da9c25
Resolved merge conflicts
ba74473
@felixbarny
Merge remote-tracking branch 'origin/master' into improvement/capture…
3762a0a 
…-request-body
@felixbarny
Wrap ServletInputStream
643dc3c 
The advantage of this approach is that we don't have to instrument
java.io.InputStream which would affect much more than just the
HttpServletRequest body.

We only create a wrapper in case we want to capture the body so the
effect on requests with disabled body capturing is minimal.
@felixbarny
Merge remote-tracking branch 'origin/master' into improvement/capture…
bca2b1d 
…-request-body
@felixbarny
Make capture_body content types configurable
00b3544
@felixbarny
End reading if reset is called
d366064 
Remove call to readNBytes as it might throw an error if eagerly linked
on Java 7
@felixbarny felixbarny requested a review from eyalkoren February 21, 2019 10:51
@codecov-io
Copy link

codecov-io commented Feb 21, 2019 •
edited
Loading

Codecov Report

Merging #498 into master will decrease coverage by 0.64%.
The diff coverage is 33.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #498      +/-   ##
============================================
- Coverage     64.32%   63.67%   -0.65%     
  Complexity       68       68              
============================================
  Files           175      178       +3     
  Lines          6548     6687     +139     
  Branches        753      765      +12     
============================================
+ Hits           4212     4258      +46     
- Misses         2082     2171      +89     
- Partials        254      258       +4
Impacted Files Coverage Δ Complexity Δ
...t/servlet/helper/InputStreamFactoryHelperImpl.java 0% <0%> (ø) 0 <0> (?)
...let/helper/RecordingServletInputStreamWrapper.java 0% <0%> (ø) 0 <0> (?)
.../co/elastic/apm/agent/matcher/WildcardMatcher.java 92.66% <0%> (-2.63%) 0 <0> (ø)
...co/elastic/apm/agent/servlet/ServletApiAdvice.java 7.46% <0%> (ø) 0 <0> (ø) ⬇️
...tic/apm/agent/impl/context/TransactionContext.java 100% <100%> (ø) 0 <0> (ø) ⬇️
...lastic/apm/agent/impl/transaction/Transaction.java 73.77% <100%> (+0.43%) 0 <0> (ø) ⬇️
...java/co/elastic/apm/agent/impl/transaction/Db.java 59.45% <100%> (ø) 0 <0> (ø) ⬇️
...servlet/RequestStreamRecordingInstrumentation.java 47.61% <47.61%> (ø) 0 <0> (?)
...ava/co/elastic/apm/agent/impl/context/Request.java 66.66% <47.82%> (-6.42%) 0 <0> (ø)
.../apm/agent/report/serialize/DslJsonSerializer.java 81.84% <57.14%> (-0.23%) 0 <0> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9379f5f...77e1f4c. Read the comment docs.

@felixbarny felixbarny mentioned this pull request Feb 21, 2019
Closed
7 tasks
@felixbarny
Copy link
Member Author

felixbarny commented Feb 21, 2019

Also tested it with a sample spring boot application:

screen shot 2019-02-21 at 16 19 20

eyalkoren
eyalkoren reviewed Feb 27, 2019
View reviewed changes
Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Very nice!
Unrelated, but looks important - shouldn't Request#charBufferPool be static 😮?

@felixbarny
Copy link
Member Author

felixbarny commented Mar 4, 2019

@nowachri-de could you sign the CLA? Do you know why the commits are from d4294 as opposed to nowachri-de?

@felixbarny
Copy link
Member Author

felixbarny commented Mar 4, 2019

@nowachri-de Maybe you need to register the email address you have configured for git in GitHub (see https://github.com/elastic/apm-agent-java/pull/498.patch).

@felixbarny felixbarny self-assigned this Mar 5, 2019
@felixbarny felixbarny merged commit f265d60 into elastic:master Mar 22, 2019
@felixbarny felixbarny deleted the improvement/capture-request-body branch March 22, 2019 08:08
felixbarny added a commit that referenced this pull request Mar 22, 2019
@felixbarny
Add changelog for #498
b7d6b96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@eyalkoren eyalkoren eyalkoren approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@felixbarny felixbarny

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Capture body of text-based requests

4 participants

@felixbarny @codecov-io @eyalkoren @alvarolobato