✨ Capture HTTP bodies on Undertow

ryandens commented 3 years ago

✨ Capture request bodies on Undertow

Background

Undertow is a web server written in Java
It's leveraged by/integrated with several large projects including Wildfly and RESTEasy.
Projects that build on top of Undertow often leverage its Java EE Servlet API Implementation
Its implementation of the Servlet API does not always leverage the Servlet API javax.servlet.http.HttpServletRequest.getInputStream()

Overview

✨ Adds Instrumentation to associate org.xnio.channels.StreamSourceChannel instances with a SpanAndBuffer instance to give the proper context
✨ Adds instrumentation to copy data from a ByteBuffer after it is read from an org.xnio.channels.StreamSourceChannel into a ByteBuffer and associate that data with the request body Span attribute.

Testing

✅ Adds a test case that reproduces a situation where our existing implementation previously did not capture request body on an Undertow deployment that we would otherwise expect to work on
Manual tests on a Wildfly servlet container with a RESTEasy JAX-RS deployment

Checklist:

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

Documentation

No new documentation is required, as we broadly claim Java Servlet support of which this undertow use-case is an implementation. This just happens to be a corner case.

ryandens commented 3 years ago

Undertow support should be added to the readme.

I'm avoiding adding undertow support to the readme because we specifically only will only fully work on Undertow servlets. Due to the implementation details of APIs like io.undertow.servlet.spec.HttpServletRequestImpl.getParameterMap(), we know that the request body won't always be accessed via the standard servlet body access APIs getReader and getInputStream() so we add this as a way to support servlets running on Undertow, but it does not mean that we support running on a raw Undertow HTTP server ( we don't).

Should this instrumentation capture headers as well?

Theoretically, we could add that over time, but the goal of this PR is to solve the request bodies missing when running undertow with Servlets, one of our supported use cases. Right now, adding full-out undertow support isn't our top priority

This is the first servlet container instrumentation in this project. Could the payloads be captured twice? E.g. first in Undertow and then in the servlet instrumentation?

I'm really glad you asked this! This was indeed happening when you brought this up. The solution I came up with was to add some lightweight instrumentation to undertow-servlet to mark and store in the instrumentation context whether or not an HttpServerExchange should be instrumented with undertow specific body-capturing.

ryandens commented 3 years ago

Hey @pavolloffay, thanks for taking the time to review these changes! Do you have any further comments/concerns about this PR?

ryandens commented 3 years ago

Why there are 3 gradle modules? Any real justification?

There is:
* common

* undertow

* undertow-servlet
Does the undertow instrumentation work without servlet? I guess the servlet depends on the core so if the core is not reused I would merge them together.

Yes, the common module is depend on by both the undertow module and the undertow-servlet module. The undertow module works without the servlet module because we can't count on undertow-servlet always being used. The undertow-servlet insrumentation module serves to add instrumentation to undertow-servlet APIs in a way that can be detected by our undertow-core instrumentation. This allows us to intelligently avoid capturing the reqeust body twice depeneidng on on what APIs are used to access it/in what context.

hypertrace / javaagent